[93705] trunk/dports/devel/ace
Ryan Schmidt
ryandesign at macports.org
Mon May 28 17:35:14 PDT 2012
On May 28, 2012, at 17:16, Thomas Lockhart wrote:
> Thanks for the feedback. Most decisions on this one are mine, with examples taken from other ports.
>
> Detailed clarifications and questions follow...
>
> On May 28, 2012, at 16:30, pixilla at macports.org wrote:
>>> Revision: 93705
>>> https://trac.macports.org/changeset/93705
>>> Author: pixilla at macports.org
>>> Date: 2012-05-28 14:30:35 -0700 (Mon, 28 May 2012)
>>> Log Message:
>>> -----------
>>> devel/ace:
>>> - Maintainer update. Closes #34658
>>> - Fix +universal variant.
>>> - Add +ssl variant.
>>>
>>> Modified Paths:
>>> --------------
>>> trunk/dports/devel/ace/Portfile
>>> trunk/dports/devel/ace/files/patch-include-makeinclude-platform_macros.GNU.diff
>>>
>>> Modified: trunk/dports/devel/ace/Portfile
>>> ===================================================================
>>> --- trunk/dports/devel/ace/Portfile 2012-05-28 21:01:38 UTC (rev 93704)
>>> +++ trunk/dports/devel/ace/Portfile 2012-05-28 21:30:35 UTC (rev 93705)
>>> @@ -2,11 +2,16 @@
>>> # $Id$
>>>
>>> PortSystem 1.0
>>> -PortGroup muniversal 1.0
>>>
>>> +# Disable the full universal support since ACE already does most of this
>>> +# and the extra stuff in muniversal (including making two build trees
>>> +# and using libtool to merge libraries) does not work for this case.
>>> +# PortGroup muniversal 1.0
>> Use of the muniversal portgroup is unusual, and is only added to the small minority of ports that cannot build universal using the standard universal variant. Thus removing the muniversal portgroup doesn't require leaving a paragraph of justification in the portfile. If use of the muniversal portgroup was not appropriate for this port then I don't know why it got added in the first place but feel free to just remove it.
> It was not clear to me that muniversal is unusual since I had taken this from another portfile in the tree. So I was leaving this fingerprint so someone else does not waste as much time as I did trying to figure out why "port install" was building two complete trees then slamming them together in spectacular and unsuccessful style.
I suppose the problem here is that you didn't know what the muniversal portgroup does, and it was added to the port before you became its maintainer, so you didn't question its presence in the portfile. It was added to the port in r79863 concurrent with updating the port to version 6.0.3, and apparently at that time, it allowed the universal build to complete successfully. Subsequent versions of ace presumably changed the build system in a way that made that method no longer work.
While I appreciate that you may want additional documentation explaining why a port does what it does, adding comments to a port explaining why it doesn't use the muniversal portgroup isn't a good idea. 99% of our ports don't use the muniversal portgroup; it wouldn't be sensible to add comments to all of them explaining why they don't do this, since we wouldn't expect them to do so in the first place. Instead we should provide good documentation in a central place (like the guide) of what the muniversal portgroup is, what it does, when it should be used. The request to add this documentation is here:
https://trac.macports.org/ticket/32428
Certainly if a portfile does something unusual or tricky, that deserves a special comment in the portfile. But ports don't need comments explaining how normal aspects of MacPorts work; that just needs to be documented in the guide or discussed on the macports-dev mailing list if there are questions.
So really, that means the opposite deserves a comment: since using the muniversal portgroup is unusual, ports that do so should have comments explaining why the normal universal variant was insufficient. Three or four years ago we went a little overboard, adding the muniversal portgroup to many ports with no justification other than the belief that it was always a good idea to do so; since then we've discovered that more often than not it just causes more problems, so if we find the muniversal portgroup added to a port and no justification in the commit message explaining why, we tend to remove it.
The commit messages by the way are a great resource for figuring out why things in a port or other file are the way they are. "svn log", "svn blame" and "svn diff -c" are your friends.
>>> +variant ssl description {Enable SSL} {
>>> + depends_lib port:openssl
>>> +}
>> This overwrites the port's global library dependency on perl, which you don't want to do. To avoid this, you need to append to the dependencies, not overwrite them. This is a good idea even if a port currently doesn't have any globally declared dependencies, to future-proof the variants against a time when global dependencies might be added to the port.
> OK, thanks.
>>> +universal_variant yes
>>> post-patch {
>>> + if {[variant_exists universal]&& [variant_isset universal]} {
>> Does the universal variant exist or doesn't it? As the port author, you should know. Since you set "universal_variant yes", it does exist, so you don't need to check for its existence.
> Haven't a clue. Taken from another portfile assumed to be a decent example. There are portfiles with and without the check for existence, and it is not clear from any documentation why or which usage might be preferred.
I only find about a dozen ports (out of over 9000) using "variant_exists" so I'll take a look and see if any of them can have it removed.
One port that I know uses it intentionally is oracle-instantclient, a very unusual port that installs pre-built binaries (since Oracle does not provide source code for it). And the binaries they provide for Intel require Leopard while their PowerPC binaries still work on Tiger, so a PowerPC/Intel universal binary could not be created, at least not on Tiger, which is why the port disables the universal variant on PowerPC but allows it on Intel.
"variant_exists" is commonly used in portgroups. This is because by the time the portgroup is included in the very first lines of a portfile, it doesn't know yet what variants the port might define.
The nonintuitive aspect of variants is that "variant_isset foo" will be true if the user requests the foo variant, even if the port does not define a foo variant. Therefore if you're writing code that doesn't know what the portfile does (i.e. in a portgroup, or even sometimes in MacPorts base) you have to check for the existence of a variant in addition to checking whether the user set the variant.
>>> + # do not set universal=1 which tries i386 and PPC but fails
>>> + reinplace "s|universal=0|universal=0|g" \
>>> + ${worksrcpath}/include/makeinclude/platform_macros.GNU
>>> + }
>> This line replaces a string with itself, and is thus completely pointless.
> Not intended as useless. This is a fingerprint to make sure that someone else does not waste time setting this parameter, which a naive person might assume is necessary for building a universal binary. It is a relic which tries to support the i386/ppc combo and breaks the build.
>
> Any suggestions for alternatives on how to document this? I've noticed that I would have liked additional comments and hints in the Portfiles I've been trying to fix or using as examples.
Adding comments in the portfiles is fine, but adding code that does nothing is confusing; I assume every line of code in a port is intentional and does something useful.
Note especially, a no-op reinplace will cause a warning message to be printed, if I ever commit the patch attached to #15514.
>>> + if {[variant_exists ssl]&& [variant_isset ssl]} {
>> You defined the ssl variant earlier, so you know that it exists; you don't need to test whether it exists.
> See above. Usage is taken from other (apparently non-optimal) portfiles ;)
>> It makes it easier to understand a portfile if all the code relating to a variant is inside the variant definition. For example:
>>
>>
>> variant ssl description {Enable SSL} {
>> depends_lib-append port:openssl
>> post-patch {
>> reinplace "s|ssl=0|ssl=1|g" \
>> ${worksrcpath}/include/makeinclude/platform_macros.GNU
>> }
>> }
>>
>>
>> variant universal {
>> post-patch {
>> reinplace "s|buildbits=64|buildbits=universal|g" \
>> ${worksrcpath}/include/makeinclude/platform_macros.GNU
>> }
>> }
>>
>>
>> Incidentally this patch in the universal variant makes clear that, without the universal variant, the port would build for 64-bit. This is not necessarily what we want. When not building universal, inspect the build_arch variable to see what arch the user requested and set buildbits accordingly.
>>
>>
> OK, will open a new ticket incorporating these suggestions. Thanks for taking the time to improve it!
>
> - Tom
I should mention here as well, "buildbits=universal" doesn't mean anything to me either. What does it mean to ace? Especially: how does the portfile communicate the value of the ${configure.universal_archs} variable to the build system? I'm concerned that it does not, and that therefore the port does not build the specific combination of architectures the user requested (via the universal_archs setting in macports.conf).
More information about the macports-dev
mailing list