Opened 8 years ago

Closed 6 years ago

#50288 closed update (fixed)

UPDATE: gpsd-3.16

Reported by: fhgwright (Fred Wright) Owned by: ryandesign (Ryan Carsten Schmidt)
Priority: Normal Milestone:
Component: ports Version:
Keywords: haspatch Cc: mojca (Mojca Miklavec), g5pw (Aljaž Srebrnič), mp@…
Port: gpsd

Description

This is just a placeholder for now. I'm in the process of testing a 3.16 port, and will upload files when ready.

Attachments (7)

Portfile (7.9 KB) - added by fhgwright (Fred Wright) 8 years ago.
Complete Portfile for 3.16_0
Portfile-gpsd.diff (9.2 KB) - added by fhgwright (Fred Wright) 8 years ago.
Portfile diff relative to 3.14_4
leopard-IPV6_TCLASS.patch (1.3 KB) - added by fhgwright (Fred Wright) 8 years ago.
Patch for IPv6 on Leopard
patch-SConstruct.diff (8.2 KB) - added by fhgwright (Fred Wright) 8 years ago.
Patch for SConstruct build script
patch-driver_rtcm2.c.diff (1.3 KB) - added by fhgwright (Fred Wright) 8 years ago.
Patch for endianness check on 10.5 (and earlier)
gpsd-3.16_0.tgz (5.9 KB) - added by fhgwright (Fred Wright) 8 years ago.
Complete tarball of gpsd-3.16_0
Port-gpsd.patch (23.0 KB) - added by fhgwright (Fred Wright) 8 years ago.
Complete patchfile for 3.14_4->3.16_0 update

Download all attachments as: .zip

Change History (26)

comment:1 Changed 8 years ago by mojca (Mojca Miklavec)

Cc: mojca@… added

Cc Me!

Changed 8 years ago by fhgwright (Fred Wright)

Attachment: Portfile added

Complete Portfile for 3.16_0

Changed 8 years ago by fhgwright (Fred Wright)

Attachment: Portfile-gpsd.diff added

Portfile diff relative to 3.14_4

Changed 8 years ago by fhgwright (Fred Wright)

Attachment: leopard-IPV6_TCLASS.patch added

Patch for IPv6 on Leopard

Changed 8 years ago by fhgwright (Fred Wright)

Attachment: patch-SConstruct.diff added

Patch for SConstruct build script

Changed 8 years ago by fhgwright (Fred Wright)

Attachment: patch-driver_rtcm2.c.diff added

Patch for endianness check on 10.5 (and earlier)

Changed 8 years ago by fhgwright (Fred Wright)

Attachment: gpsd-3.16_0.tgz added

Complete tarball of gpsd-3.16_0

comment:2 Changed 8 years ago by fhgwright (Fred Wright)

Here are the files for the 3.16 port. I supplied both a diff of the Portfile against the previous published version, and the complete new Portfile, which is more readable (and smaller). The patch files are presented as is, rather than readability-challenged diffs of diffs, and I also included a complete tarball of all the files with correct timestamps (though the repository doesn't seem to be set up to preserve timestamps).

Changes from a file perspective:

The two pc.in patchfiles are obsoleted by upstream fixes, and have been removed.

The two remaining code patchfiles have been updated to match the updated sources, but are functionally unchanged (other than adding a comment line to the rtcm2 patch to clarify why the order matters).

The SConstruct patchfile has been updated both because of substantial upstream changes to SConstruct since 3.14, and to add a couple of tweaks.

The Portfile has changed substantially.

Changes from a functional perspective:

The universal variant has been disabled, since it didn't work (and would probably require significant work on the SConstruct script to implement).

A new "xgps" variant has been added to determine whether the xgps and xgpsspeed X11 programs are included. This is because they both require gobject and gtk, expanding recursively to over 150 (previously unexpressed) dependencies that aren't otherwise needed. This is plumbed via a new SConstruct option.

There are now three Qt-related variants - qt4, qt5, and qt. The first two build against the specified Qt version, and the last is equivalent to whichever qtN variant is appropriate for the OS version. This is qt4 for OSX < 10.8 and qt5 for OSX >= 10.8, corresponding (mostly) to the Qt5 supported OS list. Qt5 doesn't consider 10.11 to be a supported OS, but this is presumably temporary, and isn't reflected in the selection. This is similar to what was suggested in #47739, though with a more aggressive move to Qt5. Since the only Qt module used here is QtNetwork, and since none of the items listed as incompatible changes in QtNetwork appears to be used, it's reasonable to expect that the Qt5 substitution will work without any code changes. However, the name change from QtNetwork to Qt5Network required adding a new option to SConstruct.

There's a workaround for #50347 in the Qt4 setup.

The variants now correctly control the build options and the dependencies, which wasn't entirely the case previously.

The "LIBPATH='.'" arguments in the SConstruct script, which only served to break linking against libdbus-1, have been patched out.

The test phase is now implemented, running all the provided regression tests (with a hack to keep them from being ridiculously slow - see the comment).

Because 3.16 changes all the Python shebang lines to refer to "python2", this now explicitly depends on python2_select, and includes both a note to suggest using it if "python2" isn't found in the command path, and a similar fatal error at the start of the test phase (since the tests require "python2").

The bad permissions on gpsprof (in the 3.16 release tarball) are corrected.

The long description has been expanded and cosmetically cleaned up.

Testing:

Build/installs have been tested for no variants, maximal variants, and the non-default Qt variant, as well as running the regression tests. This was done on a Mac Pro running 10.9.5, a PowerBook running 10.5.8, and x86_64 VMs running all OS versions from 10.5 through 10.11. The Qt5 builds failed on 10.5 and 10.6, due to the Qt5 build itself (intentionally) failing, but everything else worked. Note that Qt wasn't tested beyond testing the builds, since there aren't any Qt tests included.

comment:3 Changed 8 years ago by fhgwright (Fred Wright)

Ping.

BTW, I only belatedly ran "port lint" with the "--nitpick" option, and it complained about the naming of "leopard-IPV6_TCLASS.patch". I'm disinclined to fix this, since:

1) That's a preexisting patch file (even predating 3.14 AFAICT) that I merely updated without renaming it.

2) That fix has now been accepted upstream, so that patch (as well as the rtcm2 patch) will be obsolete in future versions.

comment:4 Changed 8 years ago by mf2k (Frank Schima)

Cc: ryandesign@… removed
Keywords: haspatch added
Owner: changed from macports-tickets@… to ryandesign@…
Version: 2.3.4

comment:5 Changed 8 years ago by fhgwright (Fred Wright)

Ping again.

FYI - a summary of the affected files is as follows:

MacPro:macports-trunk fw$ svn status
M       dports/net/gpsd/Portfile
M       dports/net/gpsd/files/leopard-IPV6_TCLASS.patch
M       dports/net/gpsd/files/patch-SConstruct.diff
M       dports/net/gpsd/files/patch-driver_rtcm2.c.diff
D       dports/net/gpsd/files/patch-libgps.pc.in.diff
D       dports/net/gpsd/files/patch-libgpsd.pc.in.diff

If Subversion had a way to generate a "commit file", I'd post it.

comment:6 Changed 8 years ago by g5pw (Aljaž Srebrnič)

Cc: g5pw@… added

Wow, that's great work! Just a small nitpick: there's no need to explicitly depend on python2_select since python2.7 has a runtime dependency on it. With that corrected I'll happily commit it.

Another thing: if you cd in the port directory and run svn diff > port.patch svn should output a patch of everything in one, making applying it easier.

Changed 8 years ago by fhgwright (Fred Wright)

Attachment: Port-gpsd.patch added

Complete patchfile for 3.14_4->3.16_0 update

comment:7 in reply to:  6 Changed 8 years ago by fhgwright (Fred Wright)

Replying to g5pw@…:

Wow, that's great work! Just a small nitpick: there's no need to explicitly depend on python2_select since python2.7 has a runtime dependency on it. With that corrected I'll happily commit it.

I thought about that, but I disagree, since I think it's bad practice to rely on an indirect dependency for something which is a direct requirement.

For example, if I have a C program that uses something in foo.h, it will of course include foo.h. If foo.h needs something from bar.h, it will include bar.h. But if my code also *directly* uses something from bar.h, then it should also include bar.h explicitly, rather than relying on the internal include within foo.h.

In this case, since the Python gpsd code needs a python2 command, it's appropriate for it to explicitly depend on python2_select (which is imperfect in that it doesn't guarantee that python2_select has been *used*, but it's the best we can do with dependencies). And strictly speaking, there's no reason in principle why python27 needs a dependency on python2_select, since it probably has no python2 commands of its own. That dependency is presumably only there for convenience, given that it's fairly inexpensive, making it particularly dangerous to rely on it.

Once gpsd's Python code has been updated to be compatible with Python 3, the python2 issue will become moot.

Another thing: if you cd in the port directory and run svn diff > port.patch svn should output a patch of everything in one, making applying it easier.

I've attached a diff of that form.

comment:8 Changed 8 years ago by mojca (Mojca Miklavec)

While what you say about bar.h is true, depending on python2_select doesn't guarantee you in any way that the python2 command exists. The package needs to reinplace the lines calling python2 with ${prefix}/bin/python2.7. You should not ask users to run port select python2 manually.

comment:9 in reply to:  8 Changed 8 years ago by fhgwright (Fred Wright)

Replying to mojca@…:

While what you say about bar.h is true, depending on python2_select doesn't guarantee you in any way that the python2 command exists.

Hence the (dynamically conditional) port notes when it doesn't (and the error if one tries to run port test without python2).

The package needs to reinplace the lines calling python2 with ${prefix}/bin/python2.7. You should not ask users to run port select python2 manually.

While I agree that that would be more convenient for users that haven't previously used port select python2, it does have some disadvantages:

1) It requires patching 8 more files that otherwise don't need it. I'm trying to *reduce* the number of MacPorts-specific patches, not increase it. :-)

2) It unnecessarily breaks the ability to use Python 2.6. The only reason I made the port dependency specifically on python27 is that I didn't want to go to the trouble of creating Python version variants (and also that the required scons port depends specifically on python27, presumably for a similar reason); the code itself will work just fine if python2 maps to python26.

3) Any user interested in PEP0394 compliance should run port select python2, anyway.

comment:10 Changed 8 years ago by mojca (Mojca Miklavec)

I do sympathize with this to some extent, but it's against the policy and general practice of most other ports.

Running reinplace is simple enough and doesn't require writing explicit patches. For example:

fs-traverse f ${worksrcpath} {
    if {[string match "*.py" "$f"]} {
        reinplace "s|/usr/bin/env python2|${python_bin}|" $f
    }
}

On top of that, if the user indeed picked python2.6 with port select and gpsd depends on py-pygtk, but the port only installed py27-pygtk, it will fail to work because the port doesn't guarantee in any way that py26-pygtk is present. Worse yet, let's assume that user has selected port select python2, but has another instance of python2 binary that comes before python provided by MacPorts. It would fail just as well because that other python2 won't have the required dependencies.

I would suggest you to raise the question on the macports-dev mailing list and ask other developers for opinion.

comment:11 in reply to:  10 Changed 8 years ago by fhgwright (Fred Wright)

Replying to mojca@…:

I do sympathize with this to some extent, but it's against the policy and general practice of most other ports.

It's difficult to quantify "most" without some fairly painstaking analysis, but it's certainly not "all".

Running reinplace is simple enough and doesn't require writing explicit patches. For example:

fs-traverse f ${worksrcpath} {
    if {[string match "*.py" "$f"]} {
        reinplace "s|/usr/bin/env python2|${python_bin}|" $f
    }
}

A patch is still a patch, whether it's by a patchfile or inline in the Portfile. Every patch makes the port harder to maintain by requiring more analysis to move to a new upstream version. As it is (since I've now gotten most of the patches incorporated upstream), there's a fairly good chance that by the time 3.17 is released, it can be completely patch-free, and I'd prefer to keep it that way.

Not to mention all the testing (see above) that I'd have to redo if I changed anything. :-)

On top of that, if the user indeed picked python2.6 with port select and gpsd depends on py-pygtk, but the port only installed py27-pygtk, it will fail to work because the port doesn't guarantee in any way that py26-pygtk is present. Worse yet, let's assume that user has selected port select python2, but has another instance of python2 binary that comes before python provided by MacPorts. It would fail just as well because that other python2 won't have the required dependencies.

That's no different from what happens with a program that specifies python and one does a port select python without having the required dependencies. And putting another python or python2 ahead of the MacPorts position in the command path breaks the corresponding selector entirely, regardless of which one it is. So none of that has anything to do with python2 vs. python.

Just among the ports I have installed here, I count 13 non-gpsd executables specifying the generic python in the shebang line (which is fine by those of us who find gratuitous monoversionism annoying). Up through version 3.15, gpsd also used the generic python in all its shebang lines. It changed them to python2 in 3.16 because at least one Linux distro has switched to Python3 as the default Python, and the code isn't Python3-ready. The only *real* difference between python2 and python is that Apple provides the latter regardless of MacPorts, while the former is missing from a base install.

Assuming /usr/local/bin exists and is in the command path, then sudo ln -s /usr/bin/python /usr/local/bin/python2 is sufficient to make python2 shebang lines work just as well as python shebang lines in all cases (at least until such time as Apple moves to a Python3 default, which hasn't happened yet, and which ought to be accompanied by a proper python2 if and when it happens). It's not a bad idea to do that for consistency anyway, regardless of what one does with python2_select, but I thought it was best to warn users who hadn't set up any form of python2 (though some gpsd users might not even care about the Python components; it's primarily a C package).

Although gpsd may be the first port where this issue has appeared (or at least the first one where anyone has noticed), it's almost certainly not the last, as additional non-Python3-ready code gets tweaked to defend against Python3 as the default. Does it really make sense to inflict a growing number of maintainability-reducing patches on a growing number of ports, just to let some subset of users avoid a one-time use of a one-line command that they should probably be doing anyway?

BTW, it would probably be a good idea to expand the port notes for the Python 2.x ports to suggest using port select python2, in addition to the current suggestion for port select python (given the likely future increase in the use of python2). If that had been in place for a while, I wouldn't have bothered doing anything about it here.

comment:12 Changed 8 years ago by mojca (Mojca Miklavec)

Please bring that discussion up on the macports-dev mailing list. In this particular ticket it's just me and you (and two others). The wider community of developers won't read the ticket unless someone brings it up on the mailing list. (You may explain (copy-paste) your thoughts again in the email rather than just pointing to the ticket.)

comment:13 Changed 8 years ago by mp@…

This patch works beautifully on Mavericks. Thanks a zillion for the effort!

(I've actually spent most of today – along with several attempts during the past months – trying to fix this port myself, and just as I gave up I found this patch. That it's been available and just not committed because of some nitpicking is sort of hilarious.)

comment:14 Changed 8 years ago by mp@…

Cc: mp@… added

Cc Me!

comment:15 Changed 8 years ago by ryandesign (Ryan Carsten Schmidt)

Status: newassigned

comment:16 Changed 7 years ago by ryandesign (Ryan Carsten Schmidt)

Please remove the requirement that the user use python_select to select python2x, as Mojca already explained above.

Please don't use system to call chmod to change a file's permissions; use file attributes -permissions instead.

comment:17 Changed 6 years ago by fhgwright (Fred Wright)

This is now obsolete, since 3.17 has been released. There's a new pull request for 3.17:

https://github.com/macports/macports-ports/pull/1033

comment:18 in reply to:  17 Changed 6 years ago by fhgwright (Fred Wright)

Replying to fhgwright:

This is now obsolete, since 3.17 has been released. There's a new pull request for 3.17:

https://github.com/macports/macports-ports/pull/1033

The 3.17 PR has been merged, so this ticket is now *really* obsolete.

comment:19 Changed 6 years ago by mf2k (Frank Schima)

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.