Page MenuHomeFreeBSD

net-im/psi: Update to 1.3
ClosedPublic

Authored by arrowd on Oct 24 2017, 5:45 PM.

Details

Summary

This diff updates psi to its latest release and switches to Qt 5.

Test Plan

poudriere testport on 11.1-RELEASE/amd64.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

arrowd created this revision.Oct 24 2017, 5:45 PM
mat added inline comments.Oct 26 2017, 3:05 PM
net-im/psi/Makefile
39–40 ↗(On Diff #34294)

Please use the CMAKE_BOOL helper.

arrowd updated this revision to Diff 34359.Oct 26 2017, 3:25 PM
arrowd marked an inline comment as done.

Address comments.

mat added inline comments.Oct 27 2017, 3:41 PM
net-im/psi/Makefile
5 ↗(On Diff #34359)

Please use DISTVERSION

7 ↗(On Diff #34359)

No need to add a / here.

21 ↗(On Diff #34359)

=0 is probably wrong, it should probably be :BOOL=false

30 ↗(On Diff #34359)

What exactly is being debugged here?

arrowd updated this revision to Diff 34407.Oct 27 2017, 6:33 PM
arrowd marked 3 inline comments as done.

Address comments.

arrowd added inline comments.Oct 27 2017, 6:34 PM
net-im/psi/Makefile
5 ↗(On Diff #34359)

Sorry, I don't get it. Why I need DISTVERSION here?

21 ↗(On Diff #34359)

It was OK, actually, CMake treats 0, FALSE and NO as FALSE.

30 ↗(On Diff #34359)

Nothing, forgot to remove.

mat added inline comments.Oct 27 2017, 7:40 PM
net-im/psi/Makefile
5 ↗(On Diff #34359)

To use instead of PORTVERSION. See 5.2.2. Versions, DISTVERSION or PORTVERSION

21 ↗(On Diff #34359)

The important bit is to always set a type for variables, :BOOL, :STRING... so that cmake does not have to guess from the content what you really meant.

arrowd updated this revision to Diff 34410.Oct 27 2017, 9:17 PM
arrowd marked an inline comment as done.

Use DISTVERSION instead of PORTVERSION.

net-im/psi/Makefile
21 ↗(On Diff #34359)

CMake uses type information only in cmake-gui to display right widget. It has no effect on code semantics.

lwhsu added a subscriber: lwhsu.Nov 24 2017, 3:55 PM
lwhsu added inline comments.
net-im/psi/Makefile
5 ↗(On Diff #34359)

There is no problem with versioning:

$ pkg version -t 0.15 1.3
<

I don't understand why we need DISTVERSION here, could you point it out?

arrowd added inline comments.Nov 24 2017, 3:59 PM
net-im/psi/Makefile
5 ↗(On Diff #34359)

It was @mat suggestion, I didn't get it too, tbh.

@mat , I think this patch is good to go, except DISTVERSIONor PORTVERSION part. I can take care of this.

mat added a comment.Feb 21 2018, 9:14 AM
In D12778#302379, @6yearold_gmail.com wrote:

Ping.

The best way to get a patch committed, if you are not a committer yourself, is to open a PR with it. This, here, is a code review tool, not a place to put patches that other will need to commit.

tcberner added inline comments.May 21 2018, 7:10 PM
net-im/psi/Makefile
14 ↗(On Diff #34410)

^qca has been flavores, this should now be qca@qt5

23 ↗(On Diff #34410)

^ sort these by kind alphabetically -- I think it makes it easier to add/look for already present dependencies:

USE_QT5=         concurrentcore dbus gui multimedia network svg webkit widgets  x11extras xml \
                            buildtools_build qmake_build \
                            imageformats_run

[the last line break I can live without ^^ ]

25 ↗(On Diff #34410)

^sort these alphabetically

tcberner added inline comments.May 21 2018, 7:15 PM
net-im/psi/Makefile
14 ↗(On Diff #34410)

^ after thought: it also does not hurt in my opinion to sort the LIIB_DEPENDS alphabetically. -- same for the USES=

arrowd updated this revision to Diff 42855.May 22 2018, 7:47 PM
arrowd marked 4 inline comments as done.

Address comments.

tcberner accepted this revision.May 23 2018, 3:54 AM

Other than the little nitpick about USE_QT5= it looks fine to me.

net-im/psi/Makefile
24 ↗(On Diff #42855)

as mentioned above, I would not mix the _build and _run only dependencies into the others.

This revision is now accepted and ready to land.May 23 2018, 3:54 AM
This revision was automatically updated to reflect the committed changes.