Page MenuHomeFreeBSD

Update libproxy to 0.4.15
ClosedPublic

Authored by tcberner on Jun 3 2018, 7:41 PM.

Details

Reviewers
kwm
Group Reviewers
gnome
portmgr
Commits
rP473062: Update net/libproxy to 0.4.15
Summary
  • Update
  • Clean up the master portfile
  • Add support for
    • python3
    • webkit2-gtk3
  • Move libproxy-gnome to libproxy-gnome2
  • I would like some input on how to do the PYTHON part properly, as it looks rather hacky at the moment.
  • libproxy-mozjs is broken on 10.4
Test Plan
  • exp-run

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

tcberner created this revision.Jun 3 2018, 7:41 PM
tcberner added a reviewer: mat.Jun 3 2018, 7:43 PM

@mat, do you have an idea how to do the python stuff nicer?

mat added a comment.Jun 4 2018, 10:45 AM

@mat, do you have an idea how to do the python stuff nicer?

Can you elaborate, what part do you want to make nicer?

tcberner added inline comments.Jun 4 2018, 10:48 AM
net/libproxy/Makefile
75 ↗(On Diff #43288)

@mat line 75--90

mat added inline comments.Jun 4 2018, 10:58 AM
net/libproxy-python/Makefile
11 ↗(On Diff #43288)

Python?

net/libproxy/Makefile
28 ↗(On Diff #43288)

This is a bit wrong, you define options, and then you say all options are excluded. The current implementation handles this by enforcing the _SLAVE before enforcing the _EXCLUDE but this could change. This should probably not exclude the current set option.

mat added inline comments.Jun 4 2018, 11:00 AM
net/libproxy/Makefile
78 ↗(On Diff #43288)

This should probably go in the Python slave port.

80 ↗(On Diff #43288)

Having two plists is probably not required, USE_PYTHON=py3kplist should handle it.

tcberner updated this revision to Diff 43302.Jun 4 2018, 11:24 AM
  • Cleanup python part
  • Don't exclude the current option.
tcberner updated this revision to Diff 43303.Jun 4 2018, 11:26 AM

.. actually exclude it

mat added a comment.Jun 4 2018, 12:13 PM

Side note, I don't really like abusing OPTIONS with a vengeance like this, I think it was better before with the LIBPROXY_SLAVE variable.

In D15655#330950, @mat wrote:

Side note, I don't really like abusing OPTIONS with a vengeance like this, I think it was better before with the LIBPROXY_SLAVE variable.

Isn't that exactly what OPTIONS_EXLUDE is for?

mat added a comment.Jun 4 2018, 1:04 PM
In D15655#330950, @mat wrote:

Side note, I don't really like abusing OPTIONS with a vengeance like this, I think it was better before with the LIBPROXY_SLAVE variable.

Isn't that exactly what OPTIONS_EXLUDE is for?

No, OPTIONS_EXCLUDE/SLAVE is for when you have "some condition" when a port only supports a subset the the master port options.
For example, most of OPTIONS_EXCLUDE_<arch>=DTRACE, or the remaining ports with a -nox11 slave port where the master port has options.

Here, in the end, none of the ports have options. Thus what I said about really abusing the options framework to gain a few helpers that are not really needed.

mat added inline comments.Jun 4 2018, 1:08 PM
net/libproxy/Makefile
42–43 ↗(On Diff #43303)

Both these should be WITH_${OPTIONS_SLAVE}.

mat added inline comments.Jun 4 2018, 1:22 PM
net/libproxy/Makefile
42–43 ↗(On Diff #43303)

Mmmm, no, only the first, but the way this is done is so convoluted that it should probably not be done this way.

tcberner planned changes to this revision.Jun 4 2018, 1:56 PM
tcberner added inline comments.
net/libproxy/Makefile
42–43 ↗(On Diff #43303)

I see, your not a fan of my cleverness ... I'll revert it to the old way

Macro failboat:

:)

kwm added a comment.Jun 4 2018, 5:59 PM

Light visual inspection only, seems mat@ already pointed out all the framework issues.

Macro lgtm:

Also feel free to axe the "$MCom:" lines, those where from the old GNOME svn development repo.

net/libproxy-kde/Makefile
5 ↗(On Diff #43303)

Don't forget to reset this one.

tcberner updated this revision to Diff 43320.Jun 4 2018, 10:03 PM

Without fancy option magic... :)
... and with working mozjs on 10.4

tcberner updated this revision to Diff 43321.Jun 4 2018, 10:22 PM

Fix CMAKE_ARGS for libproxy-webkit3.

  • Now they all should build fine on 10.4@32 and 12.0@64.
mat added a comment.Jun 5 2018, 8:04 AM

In all the slave ports, the LIBPROXY_SLAVE definition is in a bad place, it should go along the MASTERDIR definition.

Also, I think all the ${.CURDIR:H:H}/net/ might be replaced by ${.CURDIR}/../ as they are all in the net category, I find it more readable.

net/libproxy-python/Makefile
23–25 ↗(On Diff #43321)

This should happen before the target.

tcberner updated this revision to Diff 43331.Jun 5 2018, 9:14 AM

Move variables around, use CURDIR/..

mat added a comment.Jun 5 2018, 11:47 AM

There are still two .CURDIR:H:H occurrences in the master port. :-)

net/libproxy/Makefile
42–43 ↗(On Diff #43303)

Oh, no, I love clever people, but you have to remember that you are not the only one who will work on any given port. If it takes more a minute or two to understand what is happening, it could probably be done in an easier way.

In this case, now that I have thought more about these 2 lines (so more like 30 minutes of thinking) looking at it, I think using options, only the _BOOL was needed, because when the options was excluded, it would be "off" and the WITH_FOO=OFF would be added. (I may be wrong, though.)

So being clever is nice, but the more clever you think you are, the more you can create bugs that will take longer to figure out, and the more you, yourself will wonder wtf you wanted to do when you did this change 18 months ago.

tcberner updated this revision to Diff 43530.Jun 9 2018, 9:55 PM

Change to .CURDIR in masterport and fix the others.

mat added a comment.Jun 11 2018, 7:27 AM

Mmmm, I had not seen that in the master port, the CURDIR:H were before any include, and thus PORTSDIR did not exist, sorry, you should change back the CURDIR/.. with CURDIR:H :(

Other than that, looks good.

tcberner updated this revision to Diff 43864.Jun 16 2018, 5:21 AM

CURDIR:H in the masterport again.

mat added a comment.Jun 18 2018, 9:49 AM

I was looking at the LIBPROXY_SLAVES variable, and there is two places where you use it directly in its uppercased form, and three dozen where you use the lowercased version, it could probably be simplified by making its content lowercase and use :tu in the two places where an uppercase version is actually needed.

tcberner updated this revision to Diff 44022.Jun 18 2018, 6:59 PM

Use lowercase names for LIBPROXY_SLAVES

my guess is your nexst suggestion is to get rid of LIBPROXY_SLAVE and see if PKGNAMESUFFIX is set? :D

mat accepted this revision as: portmgr.Jun 18 2018, 9:47 PM
mat edited reviewers, added: portmgr; removed: mat.

my guess is your nexst suggestion is to get rid of LIBPROXY_SLAVE and see if PKGNAMESUFFIX is set? :D

Not sure, I would probably go the other way and set PKGNAMESUFFIX in the master Makefile.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 22 2018, 7:59 PM
This revision was automatically updated to reflect the committed changes.