version = "$Name$" considered harmful in .info files
Hello world, It was easy to miss this in my last message[1] to the "Announcing cvs_deploy.module" thread, so I'm starting a new one... The only reason we ever recommended people add: version = "$Name$" to the .info files they checked into CVS was for the convenience of the power-users who deploy from CVS. These people should just use the new cvs_deploy.module[2] instead. This line has caused lots of trouble and confusion for lots of people. I'm sorry I ever suggested it, and wish I had just written and released the cvs_deploy.module right when we were adding the version information to the modules page in D5. Oh well, live and learn... I've edited the 4.7.x -> 5.x upgrade docs[3] and the handbook page about writing .info files[4] to reflect the new best practice:
version (Optional) The version string will ordinarily be added by drupal.org when a release is created and a tarball packaged. However, if your module is not being hosted on the drupal.org infrastructure, you can give your module whatever version string makes sense. Users getting their modules directly from CVS will not have a version string, since the .info files checked into CVS do not define a version. These users are encouraged to use the CVS deploy [2] module to provide accurate version strings for the admin/build/ modules page for modules in directories checked out directly from CVS.
In the past (before the CVS deploy module existed), it was recommended to use this:
version = "$Name$" However, this led to much confusion and is no longer recommended. If you are a module developer, and you already have the above line in your .info files checked into the Drupal CVS repository, you should remove that line. Please let me know if you have any questions or complaints.
Thanks, -Derek (dww) [1] http://lists.drupal.org/pipermail/development/2007-June/024698.html [2] http://drupal.org/project/cvs_deploy [3] http://drupal.org/node/64279 [4] http://drupal.org/node/101009
On 6/13/07, Derek Wright <drupal@dwwright.net> wrote:
Hello world,
It was easy to miss this in my last message[1] to the "Announcing cvs_deploy.module" thread, so I'm starting a new one...
The only reason we ever recommended people add:
version = "$Name$"
to the .info files they checked into CVS was for the convenience of the power-users who deploy from CVS. These people should just use the new cvs_deploy.module[2] instead.
This line has caused lots of trouble and confusion for lots of people. I'm sorry I ever suggested it, and wish I had just written and released the cvs_deploy.module right when we were adding the version information to the modules page in D5. Oh well, live and learn...
I've edited the 4.7.x -> 5.x upgrade docs[3] and the handbook page about writing .info files[4] to reflect the new best practice:
version (Optional) The version string will ordinarily be added by drupal.org when a release is created and a tarball packaged. However, if your module is not being hosted on the drupal.org infrastructure, you can give your module whatever version string makes sense. Users getting their modules directly from CVS will not have a version string, since the .info files checked into CVS do not define a version. These users are encouraged to use the CVS deploy [2] module to provide accurate version strings for the admin/build/ modules page for modules in directories checked out directly from CVS.
In the past (before the CVS deploy module existed), it was recommended to use this:
version = "$Name$" However, this led to much confusion and is no longer recommended. If you are a module developer, and you already have the above line in your .info files checked into the Drupal CVS repository, you should remove that line. Please let me know if you have any questions or complaints.
Thanks, -Derek (dww)
[1] http://lists.drupal.org/pipermail/development/2007-June/024698.html [2] http://drupal.org/project/cvs_deploy [3] http://drupal.org/node/64279 [4] http://drupal.org/node/101009
Should we do a mass checkout and remove this line from all packages? This way we start with a clean slate now, and not wait for every developer to catch on. We have done that in the past for several things.
On Jun 13, 2007, at 3:33 PM, Khalid Baheyeldin wrote:
Should we do a mass checkout and remove this line from all packages? This way we start with a clean slate now, and not wait for every developer to catch on.
no objections here. Dries/Gerhard? -derek p.s. i just committed a few more improvements to cvs_deploy.module, so anyone who already checked it out should update their workspace. ;)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Derek Wright schrieb:
On Jun 13, 2007, at 3:33 PM, Khalid Baheyeldin wrote:
Should we do a mass checkout and remove this line from all packages? This way we start with a clean slate now, and not wait for every developer to catch on.
no objections here. Dries/Gerhard?
Be my guest if you want to spend the time. Cheers, Gerhard -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFGcIU6fg6TFvELooQRApD3AKCcyX2UsGY6zkJ1JkrZFJ3rbOEzBwCfR+0x 8yEf1iSiQeQfdDXu4YDCPOA= =r7Io -----END PGP SIGNATURE-----
On 6/13/07, Gerhard Killesreiter <gerhard@killesreiter.de> wrote:
Derek Wright schrieb:
On Jun 13, 2007, at 3:33 PM, Khalid Baheyeldin wrote:
Should we do a mass checkout and remove this line from all packages? This way we start with a clean slate now, and not wait for every developer to catch on.
no objections here. Dries/Gerhard?
Be my guest if you want to spend the time.
Cheers,
I can do the script, but someone else has to run it since I don't have access to commit them all. -- 2bits.com http://2bits.com Drupal development, customization and consulting.
I can do the script, but someone else has to run it since I don't have access to commit them all.
Here it is dww, enjoy ... #!/bin/sh for FILE in `find . -name "*.info"` do ORIG=$FILE.orig cp $FILE $ORIG cat $ORIG | grep -v "version.*=" > $FILE rm $ORIG done
On Jun 13, 2007, at 5:26 PM, Khalid Baheyeldin wrote:
#!/bin/sh for FILE in `find . -name "*.info"` do ORIG=$FILE.orig cp $FILE $ORIG cat $ORIG | grep -v "version.*=" > $FILE rm $ORIG done
or, what i was using for my own modules: perl -pi.orig -e "s/^version.*\n$//;" `find . -name "*.info"` ;) but thanks anyway... -derek
On 14 Jun 2007, at 01:10, Derek Wright wrote:
Should we do a mass checkout and remove this line from all packages? This way we start with a clean slate now, and not wait for every developer to catch on.
no objections here. Dries/Gerhard?
That's probably a good idea. -- Dries Buytaert :: http://www.buytaert.net/
On Jun 15, 2007, at 1:53 AM, Dries Buytaert wrote:
That's probably a good idea.
I checked out a whole copy of contrib to test my perl/find on it. I notice that there are a decent number of modules that do other things with version, besides $Name$. Do people think I should just nuke all version strings checked into CVS in .info files, no matter what? The only cases where it would be useful would be, for example, in HEAD, if a really careful maintainer committed something like: version = 5.x-2.x-dev to indicate that's what they were using HEAD for. Or, I noticed a tiny handful that tried to be even more careful, and commit specific version numbers: version = "5.x-1.0-alpha5" However, chances are good they'll just get out of sync and be wrong eventually, so IMHO it'll be more consistent to rely on the automated tools for this stuff. In total, find/grep/wc finds 223 .info files in HEAD that define version which don't use $Name$. Of those, 40 are incorrectly using "version = VERSION". :( Clearly, those should be purged. The easiest thing for me would be to remove everything. The docs already indicate that if you host your module on d.o, you just shouldn't commit a version line to your .info files. People who download tarballs get accurate version strings care of the packaging scripts. People who deploy from CVS can use cvs_deploy.module to get accurate version strings from the CVS branch or tag you checked out from. The *only* cases that something committed to the .info in CVS could give a better answer than what cvs_deploy.module currently can provide is if you checkout from HEAD or DRUPAL-5 and the maintainer had done irresponsible things with the DRUPAL-5 branch that didn't follow convention, and that was really 5.x-N.* code of some sort. However, there's a relatively easy solution to this problem: http://drupal.org/node/152282 In summary, 2 questions: 1) Any feedback on #152282 ? 2) Assuming folks are happy with #152282, should I just go ahead and remove version from all .info files on all branches in the contrib repo, no matter what? Thanks, -Derek (dww)
Quoting Derek Wright <drupal@dwwright.net>:
On Jun 15, 2007, at 1:53 AM, Dries Buytaert wrote:
That's probably a good idea.
I checked out a whole copy of contrib to test my perl/find on it. I notice that there are a decent number of modules that do other things with version, besides $Name$.
Do people think I should just nuke all version strings checked into CVS in .info files, no matter what?
If the version variable is replaced on packaging why worry about doing anything at all with contrib modules? If version contains $Name$ when installed we should know that it was obtained via CVS. If $Name$ is harmful then something else could be suggested, like CVS. So if version is containing CVS and someone complains we can let them know that they installed it incorrectly. I don't think that anyone but the module maintainer should modify the .info file for the project. Your perl script should only modify the core modules. Earnie
On 6/16/07, Derek Wright <drupal@dwwright.net> wrote:
Do people think I should just nuke all version strings checked into CVS in .info files, no matter what?
Since developers are mostly on this list (or should be), and the packaging scripts replace the stuff anyways, publish a notice that you will nuke this in 3 days, and that should be it. If someone has a valid objection, we discuss it. -- 2bits.com http://2bits.com Drupal development, customization and consulting.
On Jun 14, 2007, at 11:53 PM, Dries Buytaert wrote:
That's probably a good idea.
Done. See http://drupal.org/node/152819 if you happen to care. All "version" definitions from all .info files on all branches of the contrib repo are now gone. All relevant handbook docs have been updated to tell people not to do this. In the future, if you check out from CVS and see "version" defined, please file an issue with the maintainer and scold them. ;) Thanks, -Derek (dww)
On Monday 18 June 2007, Derek Wright wrote:
On Jun 14, 2007, at 11:53 PM, Dries Buytaert wrote:
That's probably a good idea.
Done. See http://drupal.org/node/152819 if you happen to care.
All "version" definitions from all .info files on all branches of the contrib repo are now gone.
All relevant handbook docs have been updated to tell people not to do this.
In the future, if you check out from CVS and see "version" defined, please file an issue with the maintainer and scold them. ;)
Thanks, -Derek (dww)
Question. What is the "proper" thing to do with a module that is not in Drupal's CVS repository in the first place? I'm tempted to just say "who cares?" except that Drupal 6 will now be checking such things and complaining if it's not set correctly. So if one is doing it manually, what is correct? -- Larry Garfield AIM: LOLG42 larry@garfieldtech.com ICQ: 6817012 "If nature has made any one thing less susceptible than all others of exclusive property, it is the action of the thinking power called an idea, which an individual may exclusively possess as long as he keeps it to himself; but the moment it is divulged, it forces itself into the possession of every one, and the receiver cannot dispossess himself of it." -- Thomas Jefferson
On Jun 18, 2007, at 5:28 PM, Larry Garfield wrote:
What is the "proper" thing to do with a module that is not in Drupal's CVS repository in the first place? I'm tempted to just say "who cares?" except that Drupal 6 will now be checking such things and complaining if it's not set correctly.
Since when? You mean when update_status moves into D6? If there's no version in the .info file, update_status just tells you it's ignoring that module due to lack of information... There's also a setting where you can tell update_status to always ignore a given module for status checks. And, I'm trying to be careful when writing update_status code to gracefully degrade if the version strings aren't following our convention, as much as possible.
So if one is doing it manually, what is correct?
Doesn't really matter, but the "correct" thing would be to either: a) Just follow the d.o version numbering scheme and manually put it in your .info files. b) Deploy your own instance of project* and let it handle this for you. ;) c) Leave them blank as further proof that you're irresponsible with your forked code. ;) (j/k) ... /me shrugs -Derek
On Monday 18 June 2007, Derek Wright wrote:
On Jun 18, 2007, at 5:28 PM, Larry Garfield wrote:
What is the "proper" thing to do with a module that is not in Drupal's CVS repository in the first place? I'm tempted to just say "who cares?" except that Drupal 6 will now be checking such things and complaining if it's not set correctly.
Since when? You mean when update_status moves into D6? If there's no version in the .info file, update_status just tells you it's ignoring that module due to lack of information... There's also a setting where you can tell update_status to always ignore a given module for status checks. And, I'm trying to be careful when writing update_status code to gracefully degrade if the version strings aren't following our convention, as much as possible.
Never mind. I actually went back and read the issue in question[1] (duh) and realized it was using a different key, so it's a non-issue. [1] http://drupal.org/node/146910 -- Larry Garfield AIM: LOLG42 larry@garfieldtech.com ICQ: 6817012 "If nature has made any one thing less susceptible than all others of exclusive property, it is the action of the thinking power called an idea, which an individual may exclusively possess as long as he keeps it to himself; but the moment it is divulged, it forces itself into the possession of every one, and the receiver cannot dispossess himself of it." -- Thomas Jefferson
On Jun 18, 2007, at 6:06 PM, Larry Garfield wrote:
Ahh yes, totally different, but I definitely can see why you were confused. Any suggestions on how to document this more clearly? There's certainly an issue with the .info file docs, because the syntax and supported/expect attributes have changed between D5 and D6. Plus there's the question of D6 having .info for themes, now. There's kind of a jumble of info about .info files that needs a grand vision and cleanup. Please see this documentation issue if anyone has any bright ideas: http://drupal.org/node/137069 Thanks, -Derek
Nice :) However, does the same apply to themes? See http://drupal.org/node/137062 "...for sites that deploy Drupal directly from CVS, it helps if you commit this change to the .info file for your theme. This is also a good way to indicate to users of each theme what version of core the HEAD<http://drupal.org/handbook/cvs/introduction#HEAD>of CVS is compatibile with at any given time." On 6/19/07, Derek Wright <drupal@dwwright.net> wrote:
On Jun 14, 2007, at 11:53 PM, Dries Buytaert wrote:
That's probably a good idea.
Done. See http://drupal.org/node/152819 if you happen to care.
All "version" definitions from all .info files on all branches of the contrib repo are now gone.
All relevant handbook docs have been updated to tell people not to do this.
In the future, if you check out from CVS and see "version" defined, please file an issue with the maintainer and scold them. ;)
Thanks, -Derek (dww)
-- Regards, Johan Forngren :: http://johan.forngren.com/
On Jun 19, 2007, at 1:53 AM, Johan Forngren wrote:
However, does the same apply to themes? See http://drupal.org/node/ 137062
Yet more evidence of confusion between: version = 5.x-1.3 (the full version string, which doesn't belong in CVS) and: core = 6.x (the version of core this thing is compatible with, which should be in CVS) This has nothing to do with modules vs. themes. I purged "version" from *ALL* .info files in CVS (yes, even the 1 D6-specific theme in contrib that put version in its .info, zenzen), not "core".... That said, I should seriously consider if cvs_deploy.module can handle missing "core", too, before people start doing major D6 porting. It's possible we don't need "core = 6.x" checked into CVS, and cvs_deploy could try to figure it out if it's not there. However, that'd probably require patching how the system_modules form works in core, so I should definitely make sure any changes are done before the D6 freeze. ;) Cheers, -Derek
On Jun 19, 2007, at 2:08 AM, Derek Wright wrote:
That said, I should seriously consider if cvs_deploy.module can handle missing "core", too, before people start doing major D6 porting.
http://drupal.org/node/152923 (feature request about this for cvs_deploy.module).
However, that'd probably require patching how the system_modules form works in core, so I should definitely make sure any changes are done before the D6 freeze. ;)
http://drupal.org/node/152926 (proposal to introduce hook_alter_info () for core). Cheers, -Derek p.s. Sorry, neither are patches yet, this is mostly to solicit feedback/support for the ideas, to see if it's worth spending time on them.
Quoting Derek Wright <drupal@dwwright.net>:
On Jun 14, 2007, at 11:53 PM, Dries Buytaert wrote:
That's probably a good idea.
Done. See http://drupal.org/node/152819 if you happen to care.
All "version" definitions from all .info files on all branches of the contrib repo are now gone.
All relevant handbook docs have been updated to tell people not to do this.
In the future, if you check out from CVS and see "version" defined, please file an issue with the maintainer and scold them. ;)
What are the reprecussions to having a version string in the info file? I don't understand the CON of specifying the version string. If I specify ``version = CVS'' what is wrong with that? Earnie
On Jun 19, 2007, at 7:14 AM, Earnie Boyd wrote:
What are the reprecussions to having a version string in the info file? I don't understand the CON of specifying the version string. If I specify ``version = CVS'' what is wrong with that?
1) Sooner or later, whatever you check in yourself will become stale and wrong (since you'll forget to update it to reflect reality). No information is better than wrong information, especially since we have good tools to figure out the correct information automatically if it's not there. 2) It's potentially confusing for the vast majority of Drupal people who download packages, since "version" will be defined twice in the .info files. 3) You'll break the cvs_deploy module's logic in the one case it's necessary: when people deploy from CVS. ;) If there's data in the .info file, cvs_deploy goes with that. If you put in something bogus like "CVS" in there, it won't attempt to figure out what you really have instead, since it assumes you must have put your local modification there for a reason. 4) "CVS" is the name of a version control system and a pharmacy. It is not, and has never been, a reasonable name of a specific version of a piece of software. ;) ... -Derek
Quoting Derek Wright <drupal@dwwright.net>:
On Jun 19, 2007, at 7:14 AM, Earnie Boyd wrote:
What are the reprecussions to having a version string in the info file? I don't understand the CON of specifying the version string. If I specify ``version = CVS'' what is wrong with that?
Derek, thanks for the feedback.
1) Sooner or later, whatever you check in yourself will become stale and wrong (since you'll forget to update it to reflect reality). No information is better than wrong information, especially since we have good tools to figure out the correct information automatically if it's not there.
I agree if the version is specified as a specific version.
2) It's potentially confusing for the vast majority of Drupal people who download packages, since "version" will be defined twice in the .info files.
If version is specified in the info file you could: 1) Not output another 2) Replace the specified 3) Output one anyway I think 2) would be the choice to use.
3) You'll break the cvs_deploy module's logic in the one case it's necessary: when people deploy from CVS. ;) If there's data in the .info file, cvs_deploy goes with that. If you put in something bogus like "CVS" in there, it won't attempt to figure out what you really have instead, since it assumes you must have put your local modification there for a reason.
Good, so you chose to use option 1) above. Could an option be included to use option 2) above instead?
4) "CVS" is the name of a version control system and a pharmacy. It is not, and has never been, a reasonable name of a specific version of a piece of software. ;)
We'll just have to agree to disagree on this point. ;p The ``version = CVS'' has more documentational value to a noob that you realize though. There are some noobs, me for example, who checkout from CVS without realizing the downside of that. As a noob I might then go searching for version to discover what it meant. Earnie
On Jun 19, 2007, at 10:40 AM, Earnie Boyd wrote:
Could an option be included to use option 2) above instead?
If you file an issue about it, preferably with a patch, I'd consider it. http://drupal.org/node/add/project-issue/cvs_deploy
On 14 Jun 2007, at 12:29 AM, Derek Wright wrote:
This line has caused lots of trouble and confusion for lots of people. I'm sorry I ever suggested it, and wish I had just written and released the cvs_deploy.module right when we were adding the version information to the modules page in D5. Oh well, live and learn...
I just played around with it, and from the perspective of somebody who runs a drupal hosting company, that deploys from cvs. Is there any reason why this should not be in core? Why should we require to ship with another module to clean up an issue that core itself has. The issue I have is that we are currently using standard drupal install profiles for the hosted sites, and having to ship with this module extra would make the hosted install profiles a special case (enabling the module above everything else). Is this going to be part of Drupal 6 at least?
On Jun 16, 2007, at 8:24 PM, adrian rossouw wrote:
Is there any reason why this should not be in core?
many. to quote http://drupal.org/node/150316 (the issue i linked to in the original email in this thread, about the origin of this module): "see http://drupal.org/node/87298#comment-154604 (comment #5) for some some of my earlier thoughts on this problem. see also my meta comments at http://buytaert.net/linus-torvalds-on-git " since people rarely read what i link to, i'll give you the 1 sentence summary: the moment we commit CVS-specific code to core, we totally tie our hands a) for exactly how we're using CVS and b) that we will always use CVS for the life of that version of core.
Why should we require to ship with another module to clean up an issue that core itself has.
core doesn't have this problem at all. we already are responsible enough with core releases to maintain the "VERSION" php constant. so, *JUST FOR CORE*, we commit the following to the .info files in CVS: version = VERSION so, if you deploy core from CVS, you already get human-readable version strings on the modules page. try it right now. checkout the end of the DRUPAL-5 branch on a test site, and you'll see "5.2 dev" for the version for all of your core modules. CONTRIB SHOULD NOT DO THIS! Contrib doesn't have the same version numbering scheme (since life is more complicated in contrib-land), so contrib authors that blindly copy core's .info files are doing it wrong, as repeatedly explained in the d.o handbook [1], [2]. as i've said before, given the shoddy state of contrib, it's unreasonable to expect that the maintainers will remember to always keep the version information accurate in the .info files checked into CVS, so we need automated tools so that contrib maintainers don't have to worry about this. that's why i'm in favor of removing *ALL* version definitions from all .info files in CVS for all of contrib. core's .info files *SHOULD* keep the "version = VERSION" checked into CVS, precisely so that core doesn't have this problem on its own.
The issue I have is that we are currently using standard drupal install profiles for the hosted sites, and having to ship with this module extra would make the hosted install profiles a special case (enabling the module above everything else).
cvs_deploy.module does nothing if you're not deploying from CVS. it doesn't assume all or nothing (maybe most of your modules are downloaded from official releases, but you ended up checking out a few things you wanted to try out via CVS or something). so, there's really no harm in enabling it on non-CVS sites, if that happened to make your life easier. that said, i don't actually understand the problem you're talking about here. i don't see why you'd have to "enable [cvs_deploy.module] above everything else". you just mean that cvs_deploy doesn't live in any of the existing install profiles? take that up with the install profile maintainers if you wish. also, keep in mind this module is ONLY about a) cosmetic stuff and b) making update_status work correctly with sites deployed from CVS. that said, maybe we need a notion of nested install profiles, or at least profiles that can be installed in serial, so that someone could just setup a "CVS deploy" profile that also enables cvs_deploy, update_status, and hopefully soon includes the handy CLI script [3]. then, you could always install this one, then optionally install whatever other profile you care about. this could potentially be helpful for the install_profile_API stuff that boris was working on...
Is this going to be part of Drupal 6 at least?
no. it's a separate module precisely because i do NOT want CVS- specific code in core, exactly as i mentioned in the original post in this thread. you should have followed moshe's advice and read the issues i linked to. ;) update_status belongs in core, cvs_deploy does not. -derek [1] http://drupal.org/handbook/version-info [2] http://drupal.org/node/101009 [3] http://drupal.org/node/124661
participants (8)
-
adrian rossouw -
Derek Wright -
Dries Buytaert -
Earnie Boyd -
Gerhard Killesreiter -
Johan Forngren -
Khalid Baheyeldin -
Larry Garfield