685378 - add source data to XPI for benefit of repacker

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect)

Description

[Brian Warner [:warner :bwarner]]

dbuc and piotr have been working on the "Repacker", an AMO service which
takes XPIs generated by the Add-On SDK (either offline or through Flightdeck)
and builds a new XPI that has the low-level SDK-provided components replaced
with newer versions. The intention is to mimic what would happen if the same
top-level source code were processed by the newer version of the SDK. AMO
would like to use this service to fix the FF-compatibility upgrade pain for
developers: rather than each developer needing to re-run a new "cfx xpi" each
time there's an update, AMO should be able to do it for them, automatically.

Because the goal is to run this on AMO, and against XPIs uploaded there (not
just against Flightdeck projects), source code for the addons is not
available. The hope is to be able to reconstruct source code from the
contents of the XPI, at least enough to accomplish the repacking service.

In the short term, dbuc and piotr think it'd be enough to include the
contents of each source package's "package.json" file in the XPI somehow. One
possibility is to put the contents in a property in the (much overloaded)
harness-options.json file. We need a naming convention so they can figure out
which package goes with which pieces of data.

In the medium term, we need a solution that survives reorganizations of the
XPI structure (bug 660629). One thought I had was a "source-data.json" file,
which has a list of package-description objects, and each object includes:

 * package name
 * mapping from source-tree filename to XPI-relative filename
 * mapping from source-tree filename to contents (as a string)

The repacker could then walk the package descriptions to figure out how to
reconstruct much of the original source tree.

In the long term, I think we should rethink the repacker. Rather than using
the original "cfx xpi" command (and therefore needing to build a source
tree), let's make an explicit "cfx repack-xpi" command, which starts with the
XPI directly. That would move some of the responsibility back into the SDK
world (where we could apply the SDK's unit tests to the process to make sure
it keeps working). The repacker would need to extract the module graph from
the manifest, identify which modules were provided by some previous version
of the SDK, remove them from the manifest, then re-do the search for each one
(using just the current SDK's packages) and wire them together in a new
manifest, then emit a new XPI with a new loader (or whatever else the new SDK
provides). It would share a lot of code with the current "cfx xpi", but
wouldn't need to try to reconstruct the original source tree, and would
probably be a bit more robust against XPI layout changes if it always starts
with the manifest.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Brian: how much time do you think it'd take to implement the medium-term solution?

Comment 2

[David Mason [:dcm :dmason]]

Checked with dbuc - the Flightdeck guys would like this as soon as possible and have it as P1.

Comment 3

[Daniel Buchner [:dbuc]]

NOTE: Please also allow Builder to pass some identifying key/value pair to the package.json that will be carried over to the generated harness.json file upon XPI construction. Builder/AMO will use this to distinguish between Builder-generated and client-installed SDK-generated XPIs.

Comment 4

[Wil Clouser [:clouserw]]

Any news on this?

Comment 5

[Daniel Buchner [:dbuc]]

Brian gave an update in the meeting today, he can probably provide a timeframe for landing this change. (I thought it was a patch landing in 1.4, but don't quote me on that)

Comment 6

[Brian Warner [:warner :bwarner]]

As of yesterday, I had the patch pretty much working, but was stuck trying to find a good way to test it. As of today, since bug 660629 landed (and changed a bunch of internals), I must rework the patch, which will take a few days (and still need to figure out how to test it, another few days for that). 1.4 froze today (as usual, shortly after 1.3 released), which means it's not going to make the 1.4 train. But it should get into 1.5 without trouble.

The patch I'm working on, in addition to changing some SDK internals, has two externally-visible effects:

* a new decompile-map.json is added to the XPI, containing a mapping from XPI paths to relative on-disk source-tree paths
* a new "cfx unpack" command takes an XPI and an output directory, and writes source files into the directory, such that running "cfx xpi" from that directory ought to regenerate the same XPI. The repacker could then be built with something like:

 cfx unpack INPUT.xpi workdir
 cd workdir
 rm -r addon-kit api-utils
 cd $MAIN
 cfx xpi

(where $MAIN is the name of the top-level package. It's not yet clear how you'd compute this.. it might be available in harness-options.json somewhere, or maybe I'll need to add it to decompile-map.json somehow).

Comment 7

[Brian Warner [:warner :bwarner]]

Attachment: pointer to pull-request 301

Ok, it's finally ready. More details in the pull-request. XPI layout should be the same as before, but I'd like Alex to double-check that, since he did the latest round of cleanup there. Irakli knows best what the XPI construction process should look like, and Mark is the newest Python guy. Let me know what you think!

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 581116 [details]
pointer to pull-request 301

I added a few comments in the pull request which are "optional" from my POV, so r=me.

Comment 9

[Brian Warner [:warner :bwarner]]

Comment on attachment 581116 [details]
pointer to pull-request 301

my patch needs to be rewritten, so cancelling the r? for now. Still interested in feedback on the general approach from alexp.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 581116 [details]
pointer to pull-request 301

Feedback already given in pull request some weeks ago.
Positive feedback. My only comment is about adding data in manifest for possible future feature. It would be better to land additional manifest data only when we start or are about to start using them.

Comment 11

[Dave Townsend [:mossop]]

Making sure we look at this in our next triage meeting

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

We aren't working on this now. Re-open or re-file if we change our mind in the future.