On Jan 30, 2008, at 19:01, June Tate-Gans wrote:
On Wed, Jan 30, 2008 at 5:42 PM, Ryan Schmidt wrote:
In this diff, it's hard to see what functional changes you made,
because you also reformatted the whitespace of the file. In the
future, could you please commit whitespace changes in a separate
revision from functional changes? Thanks.
Absolutely. The trick in this case, though, was that I literally
ripped out most of the original portfile, so a diff was actually
rather insufficient to show what it was I did. I will keep this in
mind the next time I send out a revision, though -- I know how painful
diffs can be. My apologies for the additional eye-strain.
It's not necessary to set the extract.suffix to .tar.gz since that's
the default already.
Noted. I'll remove that from the portfile.
You shouldn't define a "largedict" variant, then use
"default_variants +largedict" to enable it always. This makes it
difficult for the user to disable this functionality, should they
want to. (The user can "sudo port install cracklib -largedict" but
next time they want to "sudo port upgrade" the +largedict variant
will be selected again.) Rather, you should write the port so that
this is the default functionality, and then define a "no_largedict"
variant to disable it.
Mm. I thought about this for a while before submitting the patch.
Thing is, cracklib's functionality is severely reduced without that
dictionary, and it seemed to be the upstream maintainer's intention to
include it, which is why it is the default. Your idea makes quite a
lot of sense, though, and I will include it soon. FYI: the macports
guide doesn't mention anything about the upgrade issue you mentioned
above in the variants section, which is why I didn't consider that
aspect.
The guide mentions it here:
http://guide.macports.org/#reference.variants.user-selected
"Due to a bug in the current MacPorts base default_variants shouldn't
be used at the moment as they cause problems while upgrading ports."
The largedict variant has the path /opt/local hardcoded into the
portfile. MacPorts might be installed into a different prefix so the
variable ${prefix} should always be used instead.
Greh -- thought I managed to nix most of those. I'll fix that in the
next revision. My apologies.
Thanks for the human-linting -- it was quite informative. =o)
What can I say, I like keeping an eye on the commit mails. :)
_______________________________________________
macports-dev mailing list
[email protected]
http://lists.macosforge.org/mailman/listinfo/macports-dev
