Posted by dww on April 23, 2010 at 5:31pm
10 followers
| Project: | Project |
| Version: | 6.x-1.x-dev |
| Component: | Packages |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | drupal.org distribution blockers |
Issue Summary
As part of the Install Profile Packaging initiative, once we have #779460: Allow dynamic application of patches to dependencies we're going to need support in project_package.module to record any patches applied to a given package release, and to display those patches on the release nodes.
Comments
#1
<?phpdb_query("SELECT n.nid, n.title, cu.cid FROM files f INNER JOIN comment_upload cu ON f.fid = cu.fid INNER JOIN node n on cu.nid = n.nid WHERE filepath = '%s'", $filepath);
?>
something like that would need to be used, i guess -- then we can link from the patch location back to the node/comment.
#2
That assumes all patches live on the same host as project_package is running. I think that's a bad assumption in general. In our case, it might be true, but it's not always going to be true.
I'd rather we just treated these as "external" items that have a URL and that's what we record and display (even if in the d.o case they're really "internal"). We could get fancy and do the stuff you're talking about above as a d.o-specific add-on on top of it, but mostly, we just need a list of URLs of patch files applied to each package.
#3
the problem with just displaying URLs to patch files is that those links themselves provide no reference back to the issue to which they are attached.
it's certainly easier and better than nothing to just include those URLs -- however, i do think it would be nice to have support for URLs to the issues themselves, as part of the advantage we discussed was the public visibility of the patches in the context of the issue they are addressing.
i don't think it would be a crazy uncommon use case to have some special support built into patches that are attached to issues in the same project* install. we can code this in a way that will work generically across different project* installs, it doesn't have to be d.o specific.
#4
Fair enough. But, not everyone is going to have the same requirement or use case as us, so in terms of keeping it generic, instead of assuming that each patch is attached to an issue, we could do it something like the attached patch. {project_package_patch_item} could have columns for package_nid, patch_file_url, patch_description_url, patch_nid, and patch_comment_id. Only package_nid and patch_file_url would be required, and those would make the unique key for the table. We could just live with the patch_description_url instead of splitting out the nid and cid, but it seems nice to have those as separate columns instead of always having to parse the URL in PHP to do anything interesting with it.
Thoughts?
#5
what about bagging patch_description_url and having a helper function that assembles that from the nid and cid we save?
#6
@hunmonk: then it's impossible to represent anything *other* than local issues/nodes. That's the whole point I was trying to make in #4.
#7
ah, i see what you're meaning now. yes, i guess all three would be good. having nid/cid stored would also be an easy/fast way to know if the patch is local to the site or not. and it's cheap storage.
#8
Seems like we got both maintainers agreeing and a patch. Can we get this committed and thus push along #779460: Allow dynamic application of patches to dependencies
#9
(bump). I don't have the knowledge to do this but I'd be happy to help on the drush-make side.
#10
It seems like getting #910732: RFC: PATCHES.txt format resolved and implemented in Drush Make would be a good way for the project module to grab this information after a build, particularly if it generated a patches file in the docroot that was a manifest of all patches applied.
#11
It looks like the things that we need to add to the current schema in #4 is item_id in reference to the project or item that is being patched (so that we can display that on the distro's release node.
In the same manner we're going to split this into two tables, one for projects that are packages (and roughly correspond to items in {project_package_local_release_item}) and another table that will correspond to patches on libraries. The proposed name for these tables is project_package_local_patch and project_package_remote_patch and the remote patch table will join against the table containing the list of libraries in a distro.
#12
Here's a starting patch for the schema for the tables we discussed at our sprint.
#13
And here's a stub of the API function we talked about. It needs an item_id as another param.
If we end up needing to keep the split between patches on local vs. remote items, we might need an "item type" arg or something to tell if the item_id is pointing to a local item (release nid) or a remote item. Or maybe two separate functions? They'll share a lot of common helper logic in that case. I dunno. Anyway, just uploading this as a start.
#14
Just split out the front-end aspects of this to #1365446: Add support to display patches applied to release packages so that we can brainstorm UI and screenshots over there and keep this issue focused on the backend plumbing. They're really totally separate efforts with different people who care and can contribute... I also updated http://drupal.org/community-initiatives/drupalorg/distribution-packaging accordingly.
#15
Updated patch with the function for recording patches updated, and I slightly modified the schema to include the primary on package_nid and patch_url. This is assuming that each patch can only be included in a project once, which may or may not be true (if it's not true, we would have to expand the primary to include the item_id/remote_id).
This function always overwrites any current files when run.
#16
#17
Fixed up some stale comments form copy/pasted code.
#18
This now includes a project_package_record_remote_item() function that can record remote items and returns the ID.
#19
Latest patch includes some logic to handle deletions of all release items when deleting a release.
#20
Thanks! I've split this apart into two commits, one for #779996: Add support to record and display external dependencies included in release packages and then this one:
http://drupal.org/commitlog/commit/122/8c5a7707270469c60e7fdcc5175d3825a...
I cleaned up a few minor things: trailing whitespace, comments that didn't match the code, etc. But mostly this looks great.
In terms of getting the patch info from drush make... instead of #910732: RFC: PATCHES.txt format I'd rather just parse the .make file inside the packaging script, look for all the patches, and the tell project_package about those via the new API function we've added here: project_package_record_patch().
Therefore, I'm not sure what else we need in terms of backend plumbing here in project_package. The rest of this seems like code that needs to go into the drupalorg packaging plugin that handles distro packaging, which I've split into a separate issue for the drupalorg queue:
#1366482: Fix distribution packaging plugin to record patches and libraries included in distributions
Setting this back to active for now. Let's leave it open in case we need any other support from project_package itself. Meanwhile, #1366482 is where the current action will be happening. ;)
#21
Here's basic views support for these tables. This only exposes the remote_items table as a base table and leaves both the patches tables to be used as join tables.
I only got around to testing the project_package_remote_items table and didn't test either of the patch tables yet.
#22
Tested the views stuff fairly extensively, and reviewed the code. Everything looked great, so I split the commit into 2 and pushed them both to the 779440-distro-packaging branch.
I believe we're done here, and everything else needs to happen at either of these:
#1366482: Fix distribution packaging plugin to record patches and libraries included in distributions
#1365446: Add support to display patches applied to release packages
#23
Test patch for a release of drupalorg_testing to demonstrate this functionality.
#24
Automatically closed -- issue fixed for 2 weeks with no activity.