Per the discussion in #819874: Decide on a replacement for $Id$ tags in files, the packaging scripts need to be updated to create a manifest for all files contained in the package being generated. Derek, I know you've already made arguments for this (#606592-4: Allow updating core with the update manager), and if you already have an issue open about this then please do mark this a duplicate.

I've marked this git phase 3 as I don't know that this really needs to be done before we roll out phase 2, but I'd be happy if it got reassigned to phase 2.

CommentFileSizeAuthor
#15 914284-15.packaging-manifest.patch8.46 KBdww
#15 914284-15.MANIFEST.txt17.07 KBdww
#14 914284-14.packaging-manifest.patch8.16 KBdww
#14 914284-14.MANIFEST.txt17 KBdww
#13 914284-13.packaging-manifest.patch7.86 KBdww
#13 914284-13.MANIFEST.txt16.93 KBdww

Comments

dmitrig01’s picture

dmitrig01 commented

I know this is only marginally related but this has the potential to greatly help drush make in being more intelligent about what's a project, where to put projects, how to re-make a file, etc.

dww’s picture

dww
we/he/they
commented

This might also help the Update manager. See #986616: Update Manager fails when the primary module/theme for a project lives in a subdirectory However, I think we're screwed there in the (cvs|git)_deploy cases... *shrug* No, I don't remember an older issue about this. The concept has come up numerous times, but I think this is the first time anyone actually opened a separate issue to discuss it.

chx’s picture

chx commented

I do not know what phase 3 means here but this is fairly mandatory to have the moment git is deployed. I thought we agreed on this when $Id$ got removed.

sdboyer’s picture

sdboyer commented
Issue tags: -git phase 3 +git phase 2, +git sprint 9

@chx - yikes, thanks for responding on this issue and resurfacing, this had slipped off my radar since I initially tagged it phase 3 and I haven't been watching that queue (I'm rechecking it now). Phase 2 is everything leading up to initial launch, phase 3 is everything after, and I agree that the upshot of that discussion was that we'd need to launch with this. So I'm retagging it, and we'll talk about it at tomorrow sprint/sooner.

I suspect we're not going to be able to get this done for the initial public testing launch since that's only a week away, but shortly thereafter. So I've tagged this for sprint 9, our next sprint.

mikey_p’s picture

mikey_p commented

I skimmed the other issue, is there a spec that we're trying to match with this?

sdboyer’s picture

sdboyer commented

No, we kicked around some ideas in that other thread, but never settled on anything definitive. Step 1 here is defining such a spec. In #819874-51: Decide on a replacement for $Id$ tags in files, I copied in a manifest used by gentoo's portage as an example. That's one place we could start from.

dww’s picture

dww
we/he/they
commented
Component: Packages » Releases

(I know it's terribly confusing, but project_package is actually for something completely different than releases and packaging releases). ;)

Defining the spec should definitely continue to happen. But no one should actually start writing a patch for this until the dust settles on #781246: Write Git-specific packaging plugin for d.o. So, I'm almost tempted to mark this postponed. But yeah, we need a spec, so it should remain active.

eliza411’s picture

eliza411 commented
Issue tags: -git sprint 9 +git sprint 10

Retagging to git sprint 10. this isn't going to happen this sprint.

sdboyer’s picture

sdboyer commented

Now that we've gotten #781246: Write Git-specific packaging plugin for d.o together, this is ready to be worked on. And honestly, shouldn't be too tough. Let's say the spec for this is:

(last modified commit SHA1) (file md5) (relative filepath)

Putting the relative filepath last is important, I think, since it can contain spaces; we're guaranteed the others won't. Anybody have a problem with this proposal? Have I missed something?

dww’s picture

dww
we/he/they
commented

Should there be a header with meta-data about the contents overall? URL to the Git repo it came from? Project URL? Release URL? Creation date? Anything else that would also show up in the release-history XML for that project or release?

Also, for each entry, do we want to include the date of the last commit SHA1?

sdboyer’s picture

sdboyer commented

Re: header - yeah, I'd be good with that. Commented lines (leading hash character) with anon clone URL. Project URL seems fine, too. I dunno if creation date matters that much, but I don't know the update systems.

Human-readable timestamps at the end of each line indicating the last modification time for each file would also be important. If something programmatic actually needs the data, it can always convert it to a more usable format, but a big part of the goal here is making the timestamps usable for people, per all the discussion in the original issue.

dww’s picture

dww
we/he/they
commented
Project: Project » Drupal.org customizations
Version: 6.x-1.x-dev » 6.x-3.x-dev
Component: Releases » Code

Given that this MANIFEST is going to include a Git hash, and given that we're talking about a new file created during packaging and included in the various archives, this needs to live in the d.o-specific packaging ctools plugin, not in project* itself.

Another thing that would be really helpful is if the header included *just* the project shortname. I know that's technically going to be in some of the URLs, but it'd be better not to have to parse that out of URLs and just have it directly. Probably wouldn't hurt to include the version string while we're at it.

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww
Status: Active » Needs review
StatusFileSize
new 914284-13.MANIFEST.txt16.93 KB
new 914284-13.packaging-manifest.patch7.86 KB

Here's a patch and an example of the resulting MANIFEST.txt file for project-6.x-1.x-dev.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 914284-14.MANIFEST.txt17 KB
new 914284-14.packaging-manifest.patch8.16 KB

And now with a human-readable key as the last header entry to explain WTF the payload of the file is.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 914284-15.MANIFEST.txt17.07 KB
new 914284-15.packaging-manifest.patch8.46 KB

Now with the Git SHA1 that the release itself is being generated from in the header...

dww’s picture

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

Committed #15 to DRUPAL-6--3 of drupalorg. We might want to tweak this later to deal with a few of the TODO comments I left in the code, but this is good enough for now.

chx’s picture

chx commented
Status: Fixed » Needs work

Hrm, I am not too happy with the line breaks. Can we remove them? Edit: my browser fooled me, sorry! Can we remove the timezone +0000 if it's +0000 always?

sdboyer’s picture

sdboyer commented
Status: Needs work » Fixed

Yeah, dww mentioned wanting to remove those, and it's consistent with the ISO standard, so I'm fine with doing that.

sdboyer’s picture

sdboyer commented
Status: Fixed » Needs work

Whoops, xpost.

sdboyer’s picture

sdboyer commented

Sorry for not posting this earlier, but...

On further reflection, I'd like to roll this patch back. When I tweeted about this shortly after the patch went in, Crell responded by pointing out that we've invented yet another Drupal-specific pseudo-standard here. I was (quite) annoyed initially, but couldn't really contest his basic point, so I asked him to point me to an alternative. He pointed at PECL initially, and suggested that I talk to mbutcher who'd done a fair bit with it. Turns out PECL's XML-based packaging metadata really doesn't help us at all, but Phar just might. In particular, its manifest format should be of interesting to us: http://us2.php.net/manual/en/phar.fileformat.phar.php and http://us2.php.net/manual/en/phar.fileformat.manifestfile.php .

Without much experience actually using Phar, looking through the set of options it provides, it's very tempting. It can generate tar or zip, use bzip/gzip/zlib compression, and maybe most importantly can generate phars, which is itself a potentially very useful format for distributing components (not the least for the case where you're trying to keep clueless clients from modifying files). We have no use for phars right now, but if we ever want to use them, we'll need to start using Phar. And I don't think that's an unrealistic possibility. Also, it's got built-in support for assembling a package from an Iterator instead of just a literal filesystem location, which means it'd probably be possible for a suffiicently enterprising mind to write an entirely stream-based package assembly system that means packages in their finalized form never, ever have to hit disk. And, y'know, we avoid yet more NIH.

There are other benefits, too, but those are the big ones. As for disadvantages, I see three:

- Using Phar means PHP 5.3, which our infra isn't on quite yet.
- The Phar manifest uses totally not-human-readable UNIX timestamp instead of an ISO date.
- The code itself gets a bit more complicated, especially if we go the in-memory-assembly route. I'm loathe to even really mention this as a disadvantage, though. Optimizing lower-level operations can mean avoiding lowest-common-denominator approaches, and we shouldn't hesitate to reap those benefits for a second.

The MANIFEST is an important and really useful thing, but as soon as we settle on a standard, we're pretty much stuck with it. Which is really why I'd like to pull this back out and try to be a little more deliberate about this decision than just dww and I bandying some code back and forth for 90 minutes. IMO, this really isn't a launch blocker, as we can always go back and retroactively generate packages. It's much more important that we make a good, forward-thinking decision about this.

Whatever we do, though there is an immediate problem with the patch that was committed. I can't remember if switching to the manifest meant we abandoned the decorating of .info files, but this logic:

    // Update any .info files with packaging metadata.
    foreach ($info_files as $file) {
      if (!$this->fixInfoFileVersion($file)) {
        wd_err("ERROR: Failed to update version in %file, aborting packaging", array('%file' => $file), $release_node_view_link);
        return 'error';
      }
    }

now throws errors because $info_files never gets defined. It would be great if this could get immediate attention, whether we intend to keep the manifests or not.

Crell’s picture

Crell commented

On the infra side, Phar is bundled with PHP 5.3 but can be installed on late-model PHP 5.2 as well: http://de.php.net/manual/en/phar.requirements.php

I am unsurprisingly in favor of using an existing manifest format over rolling our own. While it may be a bit more complex, this is a file that is mostly intended for automation and for minimal human checks (eg, quick glance to see if a number is bigger than another number) so that's OK. The potential benefits of leveraging an existing standard, one that was already designed to handle a variety of situations, outweigh the non-purpose-built downsides.

I have not looked into Phar itself in much detail so I cannot yet comment if that is the "right" format to use, but sdboyer's analysis above suggests that it is. It is a format designed for packaging up and distributing applications and modules, so it does seem like a natural fit. (I believe it was modeled on Java's "JAR" file format, at least in original concept if not in implementation.)

dww’s picture

dww
we/he/they
commented

I fixed the .info processing at http://drupal.org/cvs?commit=500632 (only in DRUPAL-6--3 branch due to this patch).

I don't know enough about phar yet to know if it's the right solution.

I had a clarifying chat in IRC with sdboyer, and luckily, we're on the same page about a number of things:

A) If we're going to use a phar-compliant manifest file, it's going to be b/c we're actually creating phar archives via their whole class system and getting the manifest for free.

B) It's not obvious phar is the right solution for us, that requires more research.

C) Generating archives on-demand at download time is not a win for us, and not a reason to use phar. Disk is cheaper than server CPU/RAM, especially since in this case, disk is being served to disk by separate hardware (ftp.d.o) that's optimized for the job.

D) Sam was only talking about perhaps being able to create the phar archive in RAM in the first place, instead of doing it initially via file I/O. However, again, this is basically pointless/premature optimization -- util.d.o (where packaging will be running) is not having I/O problems now. If we want to reduce file I/O during packaging, #381956: Mark project releases as "to be updated" only if pushes happen there would be a vastly more important win than in-RAM phar'ing... ;)

E) It's not a Git-launch blocker to switch to using phar for this stuff. Really, that's a separate proposal that needs time and thought, not a hasty decision to be rushed in.

-----

Things we're not sure we agree on yet:

F) I don't want the possibility of someday using something cooler to prevent us from doing something useful now. "Don't let perfect be the enemy of good" and all that. Sam characterizes this as: "i tend to look at it not as perfect enemy of the good, but how a quick solution to a not-pressing problem could obstruct much niftier things in the future". If/when we have something better, I think we could either tell people "sorry, our homebrew is being replaced by $cooler, deal with it", or we could continue to let working code work, provide our MANIFEST.txt for those that want it, and let phar or whomever else add something else next to it for folks who want that.

G) This MANIFEST file will help the Update manager in core (a lot). There are a number of problems this could help solve. So, I think this is more of a pressing problem than I think Sam does.

-----

Still leaving this as needs work to either roll-back per #20 or strip off the useless timezone per #18. However, I need more clarity on the plan before doing either... ;)

sdboyer’s picture

sdboyer commented

I'm not sure I actually agree on D), but that's OK - it's a later discussion quite separate from this one.

webchick’s picture

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

Honestly, I'd rather push this off post-launch. While I agree with G) that this will ultimately be a really awesome thing for our community, there is still some contention around the ultimate solution, and we definitely don't want to do this twice.

Let's take another look at this once the big stressful thing is over and we can have some fresh eyes, and get the larger community involved in the discussion.

sdboyer’s picture

sdboyer commented

I've reverted the two commits made here so far, so that we a) don't have the spectre of this hanging over our heads with launch (there's already plenty of things like that, tyvm) and b) aren't generating inaccurate tarballs with gd.d.o anymore.

sdboyer’s picture

sdboyer commented
Issue tags: -git phase 2 +git phase 2 leftovers

We've officially punted this to post-launch, updating tags accordingly.

bwinett’s picture

bwinett commented

Just wanted to note that there is an open security vulnerability with Phar: http://www.securityfocus.com/bid/47545/info .

  • Commit cce0e35 on 6.x-3.x, 7.x-3.x-dev by dww: 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • Commit 2807bc6 on 6.x-3.x, 7.x-3.x-dev by sdboyer:
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • Commit a92daee on 6.x-3.x, 7.x-3.x-dev by sdboyer:
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • Commit d5b2176 on 6.x-3.x, 7.x-3.x-dev by dww:
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on 2299191-beta_project_issue_project_searchapi 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on 2299191-beta_project_issue_project_searchapi
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on 2299191-beta_project_issue_project_searchapi
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on 2299191-beta_project_issue_project_searchapi
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on 2322267-migrate-country-field 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on 2322267-migrate-country-field
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on 2322267-migrate-country-field
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on 2322267-migrate-country-field
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on 2322267-migrate-gender-field 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on 2322267-migrate-gender-field
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on 2322267-migrate-gender-field
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on 2322267-migrate-gender-field
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on 2348121-missing-bio-information 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on 2348121-missing-bio-information
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on 2348121-missing-bio-information
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on 2348121-missing-bio-information
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on 2350591-not-spammer-role 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on 2350591-not-spammer-role
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on 2350591-not-spammer-role
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on 2350591-not-spammer-role
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on 2322267-bakery-sync-country 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on 2322267-bakery-sync-country
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on 2322267-bakery-sync-country
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on 2322267-bakery-sync-country
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on random-supporter-logos 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on random-supporter-logos
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on random-supporter-logos
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on random-supporter-logos
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on hosting-type-field 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on hosting-type-field
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on hosting-type-field
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on hosting-type-field
    #914284 by dww: Follow-up fix so that we properly update .info files...

  • dww committed cce0e35 on filter-partners-by-sector 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on filter-partners-by-sector
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on filter-partners-by-sector
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on filter-partners-by-sector
    #914284 by dww: Follow-up fix so that we properly update .info files...
mikey_p’s picture

mikey_p commented
Issue summary: View changes

This seems to have broken sometime somewhat recently, maybe in the Drupal 7 upgrade?

  • dww committed cce0e35 on restrict-commit-issue-notifications 
    #914284 by dww, sdboyer: Updated packaging to create a MANIFEST.txt file...
  • sdboyer committed 2807bc6 on restrict-commit-issue-notifications
    Revert "#914284 by dww: Follow-up fix so that we properly update .info...
  • sdboyer committed a92daee on restrict-commit-issue-notifications
    Revert "#914284 by dww, sdboyer: Updated packaging to create a MANIFEST....
  • dww committed d5b2176 on restrict-commit-issue-notifications
    #914284 by dww: Follow-up fix so that we properly update .info files...
drumm’s picture

drumm
he/him
Location NY, US
commented

What exactly is broken?

mikey_p’s picture

mikey_p commented

Sorry, I thought this was what also added the list of projects and versions to the release node for a distribution.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs work » Closed (won't fix)

Seems like we haven't been missing having this.