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. :)