Part of http://drupal.org/community-initiatives/drupalorg/distribution-packaging...

Even if drush make was smart enough to handle #1371298: Rewrite .info files inside drush make when checking out from git to include version and project info it probably isn't going to add a hard requirement that you always specify a version. So, I believe we're going to want some custom validation in drupalorg_drush so that projects *must* define a version (to say what branch each project is from so that the .info rewriting always works), even if they optionally specify an exact git hash instead of just taking the last commit on that branch...

CommentFileSizeAuthor
#30 1371306-30.require-branch-with-revision.patch6.61 KBhunmonk
#26 1371306-26.require-branch-with-revision.patch1.82 KBdww
#9 1371306-9.allow-full-version.patch1.94 KBdww
#2 drupalorg_drush.1371306-02.patch8.08 KBjhedstrom

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Assigned: Unassigned » jhedstrom
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Active » Needs review
StatusFileSize
new drupalorg_drush.1371306-02.patch8.08 KB

I've got this working in the allow-dev-snapshots branch. Attached is a diff for review. If this all looks good, I'll merge that branch in (all tests are passing).

hunmonk’s picture

hunmonk commented

patch looks good on visual inspection. it's a shame that the syntax for specifying a specific git hash is so much more verbose than using the latest dev tarball. although we're not requiring it, it would be nice if distro maintainers used git hashes, as it will make their releases more stable.

to that end, i filed #1375234: default download type to git when download revision key is set in the hopes of simplifying things. if that flies, then things may need to be adjusted here.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

The allow-dev-snapshots no longer merges cleanly with master now that #684788: Verify Library URLs against a White-list for drupal-org.make is merged.

That said, I'm unclear what this issue has to do with that branch. This issue here is a relatively simple change, no? Can we just handle that via a patch instead of confusing an unrelated feature branch with it? Or maybe there's some dependency with the -dev stuff I don't understand... ?

Thanks,
-Derek

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

With the latest drush, specifying a specific git hash should, at a minimum, require:

projects[foo][revision] = "HASH"

where 'HASH' is the specific git commit. With this patch, we also require that the specific branch be specified. Since d.o requires a branch, in addition to the hash, the patch can be re-rolled to require the minimum of:

projects[foo][version] = "2.x-dev"
projects[foo][revision] = "HASH"
dww’s picture

dww
we/he/they
commented

Oh right... specifying a hash only works if you're doing -dev... duh. Sorry!

Anyway, yeah, this branch is no longer merging cleanly... are you available to resolve the conflicts, or should I?

Thanks!
-Derek

hunmonk’s picture

hunmonk commented
Status: Needs work » Needs review

fixed up the allow-dev-snapshots branch to merge cleanly, and added a validation transformer that verifies that version is specified when download/revision is specified. also added tests for make and verify-makefile that exercise this validation.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

Yup, finally working nicely, now that #1371298: Rewrite .info files inside drush make when checking out from git to include version and project info is fixed in drush core. I just merged allow-dev-snapshots into master and pushed it all. Calling this fixed. I'm opening a separate infra issue about deployment.

dww’s picture

dww
we/he/they
commented
Status: Fixed » Needs review
StatusFileSize
new 1371306-9.allow-full-version.patch1.94 KB

Given how #1267228: Drush Make should use Drush core's native download abilities concurrently turned out, and given that #1404702: Fix .info file version rewriting for -dev releases still isn't done, I think we should allow .make file authors on d.o to specify either a project-level 'version' or a download-level 'full_version' to get past the validation. In some ways, just directly defining full_version (which is passed untouched into the .info files) is best for this.

Thoughts?

Thanks,
-Derek

langworthy’s picture

langworthy commented

I found this issue after commenting in #1432312: Set project-level 'version' attribute automatically if a Git 'branch' or 'tag' attribute is defined.

I'm not clear what the problem is that requires this change. #5 says "d.o requires a branch". Requires a branch for what?

dww’s picture

dww
we/he/they
commented

Because if you just define a revision, we have no way of knowing what branch you're talking about (since a given commit can live on many branches), and therefore we can't rewrite a sane version string into the .info files such that update module in core works.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I wonder if that's actually a problem. We could do one of the following, maybe?

1) Make it very clear that some of the projects are Git checkouts on the release node. User is instructed to download Git deploy.
2) Auto-package Git deploy in any profiles that come with Git checkouts.

langworthy’s picture

langworthy commented

Ah, I see how we're now making the version "LAST_TAG+0-dev". And update manager will work with that.

Cool.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

In #12 I was very, very confused. :) I was thinking we included git clones in the tarballs and the concern was about end users, but this is actually about auto-generating version = X strings in .info files of -dev releases. Ignore me!

dww’s picture

dww
we/he/they
commented

Re: #13: langworthy: better yet, it's "LAST_TAG+[N-commits-since-that-tag]-dev". For example, on d.o we just deployed views "6.x-2.16+5-dev" -- the end of the 6.x-2.x branch, 5 commits after the 6.x-2.16 official release.

Re: #14: Right. ;)

langworthy’s picture

langworthy commented

Ok. I'm still a little confused. If the goal here is to generate sane .info versions don't you need branch and not version

As #5 mentions "With this patch, we also require that the specific branch be specified."

So If I was using a recent version of views I would do 

projects[views][download][branch] = 7.x-3.x
projects[views][download][revision] = HASH
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

When this was originally being worked on, at some point we added a shorthand in make to allow one to speficy projects[foo][revision] = HASH which is the same as speficying projects[foo][download][revision] = HASH.

Similarly, the thinking with using version (top-level) was that it would be better dx. However, munging the concept of branch into version is proving problematic, so perhaps we can just go back to requiring a branch be specified at the download level?

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

A related problem is that the drupalorg_drush plugin is requiring that we load release history XML files for everything, since it's trying to get the release node IDs (which we need for other things). And, *that* code wants to use the top-level version attribute. See #1432476: drupalorg_drush always try to download release history XML info.

Anyway, I think we're over-complicating things here. Since I got #1404702: Fix .info file version rewriting for -dev releases into drush core, #9 is no longer needed, and this issue can just be called 'fixed' again. What we have works. It's only to cover an edge case where stuff being packaged on d.o needs a *specific* commit that's not a release tag. Generally, people should be packaging from official releases. If that doesn't work, they can use a revision, and in that case, they just define the version they need.

The version vs. branch confusion in this issue is only from comments made in the issue. End distro maintainers won't know/care that what we really need deep inside drush make is the branch so we can munge the .info file. The error message is clear. The docs can be made clear.

langworthy’s picture

langworthy commented

Is the correct way to do revisions:

projects[views][download][version] = 3.1
projects[views][download][revision] = HASH a few commits after 3.1 tag

Or

projects[views][download][version] = 3.x-dev
projects[views][download][revision] = HASH a few commits after 3.1 tag

?

I've been thinking we've been talking about the former but I see Commons uses the latter

dww’s picture

dww
we/he/they
commented

Neither. ;)

You need:

projects[views][version] = 3.x-dev
projects[views][download][revision] = HASH a few commits after 3.1 tag

Unless it's exactly the same HASH as the 3.1 tag, calling the version 3.1 would be a lie. It's a -dev release on the 3.x branch.

langworthy’s picture

langworthy commented

Gotcha!

I think a part of my problem is that I don't think of dev releases as "versions" but rather "some commit on a branch". So asking me to include a version was throwing me way off.

I still think [branch] = 7.x-3.x is a better fit than [version] = 3.x-dev but i'm not too worried about it.

Thanks for your patience.

dww’s picture

dww
we/he/they
commented
Assigned: jhedstrom » Unassigned
Status: Fixed » Active

Well, assuming we're going to get #1432312: Set project-level 'version' attribute automatically if a Git 'branch' or 'tag' attribute is defined. working, so long as we had 'branch' we could use that. Now's the time to clean this up and document it before people start really trying to build distros with this stuff. My only concern is that 'branch' and 'revision' sort of conflict with each other, since they're intended to be different ways to get drush make to checkout your code from Git. But maybe I'm the only one that thinks so. If this is going to be more intuitive for distro maintainers:

projects[views][download][branch] = 7.x-3.x
projects[views][download][revision] = HASH

we can go with that...

Thoughts?
-Derek

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

#22 sounds better to me, as a relative noob to this make file stuff.

It was relatively WTf-y to have to specify a project-level property, rather than a property at the same level as revision. It was doubly WTF-y that it didn't follow the same convention as a Git branch name. Since it's not possible to get around being forced to specify a version of some kind, I think 'branch' makes sense.

My only concern is that if this change is made, it must be made in both Drush Make and Drupal.org Drush. I don't want us veering off making our own .make file syntax.

dww’s picture

dww
we/he/they
commented

Hrmph. The problem with defining 'revision' and 'branch' is that currently drush make doesn't like that and assumes you should only do one or the other. So, if we go that route, we'd have to patch drush make itself, too.

dww’s picture

dww
we/he/they
commented
Title: Add validation to ensure that if a .make file includes a git hash, it also defines a version » Add validation to ensure that if a .make file includes a git hash, it also defines a branch
Status: Active » Postponed

FYI: I opened #1435156: Need to allow both revision and branch to be specified (better for version strings, matches generate-makefile output) as a bug report. I'm hoping to just fix drush make itself in that regard, then we can do the more sane thing here of requiring a branch instead of the bizzaro make-specific version string fragment.

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww
Status: Postponed » Needs review
StatusFileSize
new 1371306-26.require-branch-with-revision.patch1.82 KB

Assuming #1435156: Need to allow both revision and branch to be specified (better for version strings, matches generate-makefile output) is committed, here's a patch for this. We'll still let it slide if they just give us a 'version' attribute, although the error message just mentions 'branch'. Hope that's agreeable.

Also simplified the code for this whole class. It was copy-paste from the attribute whitelist style transformer classes, but that's not necessary here.

langworthy’s picture

langworthy commented

Thinking some more about this I've realized that as a distro developer, if I choose a revision checkout there is a reason for that and I don't want my users upgrading any modules. If I've chosen to use a revision I've most likely patched the module and upgrading would break the functionality I've put in place. It's my responsibility to put out a new release if a security update comes along.

We don't need update manager to work for dev checkouts in distros.

dww’s picture

dww
we/he/they
commented

@langworthy: That dynamic is not specific to revision or -dev. See #1425522: Help manage updates for distributions.

dww’s picture

dww
we/he/they
commented

#1435156: Need to allow both revision and branch to be specified (better for version strings, matches generate-makefile output) was just committed, so IMHO this should go in. Any final objections?

Thanks,
-Derek

hunmonk’s picture

hunmonk commented
StatusFileSize
new 1371306-30.require-branch-with-revision.patch6.61 KB

code looks good, i cleaned up the tests around this and added another for coverage. all tests passing.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

Thanks for the test improvements. I realized my patch in #26 had a minor bug in that the project name wasn't getting properly printed out in the validation error message. Fixed that, ran all the tests to ensure they're working as expected, and committed/pushed.

hunmonk’s picture

hunmonk commented
Status: Fixed » Needs work

i believe we still have a problem here... :|
given this makefile:

core = 7.10
api = 2

projects[update_test_module][version] = "3.x-dev"
projects[update_test_module][download][type] = "git"
projects[update_test_module][download][revision] = "2bfc8d9"

putting version there triggers the plugin to look for a release, which it finds, and over at #1433784: Fix how drupalorg_drush handles release history XML and propagates data to the packaging script it promptly stuffs the release nid into the package_contents file, which is wrong. so i'm not sure if specifying version in this case is a good thing. at the very least, we need to figure out how to not ask for a release nid when we're really working off a commit hash.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Fixed

I disagree with your assessment. That .make file is legit, and we should say that such a distribution includes update_test_module 7.x-3.x-dev. It's a -dev release, and it's always changing. At *some* point, it included that particular revision. Maybe there have been more commits since then and it no longer does, but that's the nature of -dev. Still, it's helpful to know that this branch of this project was included in your distro release, which is exactly what's happening. Being able to find the right -dev release is basically the reason we require that the distro maintainer define branch or version if they're defining a hash. That's the whole point (both for listing what's in the distro, i.e. the thing you're talking about, and for rewriting the .info files).

Automatically closed -- issue fixed for 2 weeks with no activity.