Closed Bug 946343 Opened 8 years ago Closed 8 years ago

Add an option to cfx to easily change addon metadata on build time

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 3 obsolete files)

I think I'm not the first addon author that has to hack package.json on build time, just before running cfx in order to tune addon version, name before building the xpi.

My current usecase is for the simulator. The addon version isn't stored in package.json. So we have to sed/replace a string in package.json in makefile script in order to set the final version, just before calling cfx xpi.
Then either you have an objdir, either you pollute your repo with unwanted modifications.

That would be really handy to be able to customize addon metadata on build time.

I'm suggesting to add a "--manifest-overload /path/to/file.json" cfx argument,
that would read a json file and overload any property of addon package.json with the properties from this file. That, without actually modifying package.json but only replacing in-memory values before crafting the xpi.

I'm open to other ways to simplify this story, as soon as it simplify the final build script.
Attached patch Implements --manifest-overload (obsolete) — Splinter Review
Attachment #8342510 - Flags: review?(rFobic)
Blocks: 944451
Comment on attachment 8342510 [details] [diff] [review]
Implements --manifest-overload

Hi Alex,

I'm delegating this to Erik since he's actually taking SDK to a state where
cfx will be finally obsolete, so future of this change maybe pretty much at
risk.

As of your use case, why not just generate package.json at build time and swap
existing one, make .xpi and swap back ? I feel like that option will be less
bound to cfx or whatever we may have in a future. But again I'll let you and
Erik decide what makes most sense here
Attachment #8342510 - Flags: review?(rFobic) → review?(evold)
(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #2)
> As of your use case, why not just generate package.json at build time and
> swap
> existing one, make .xpi and swap back ?

Mainly to avoid having to pollute your repo with such modification,
not having to maintain an objdir/stage directory.

Are we going to stop offering command line tool for addons?
That's one of the main reason why I'm using the sdk for the simulator:
easily run the addon, build the xpi (handle the install.rdf mapping from package.json)
and also handle update.rdf to easily ship updatable addon (hosted outside of AMO).

If that the case, I'd better switch immediatly to a bootstrap.js addon
and handle xpi creation manually...
(In reply to Alexandre Poirot (:ochameau) from comment #3)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #2)
> > As of your use case, why not just generate package.json at build time and
> > swap
> > existing one, make .xpi and swap back ?
> 
> Mainly to avoid having to pollute your repo with such modification,
> not having to maintain an objdir/stage directory.
> 
> Are we going to stop offering command line tool for addons?
> That's one of the main reason why I'm using the sdk for the simulator:
> easily run the addon, build the xpi (handle the install.rdf mapping from
> package.json)
> and also handle update.rdf to easily ship updatable addon (hosted outside of
> AMO).
> 
> If that the case, I'd better switch immediatly to a bootstrap.js addon
> and handle xpi creation manually...

This is a minor misunderstanding.  We will still have the `cfx init`, `cfx run`, `cfx xpi`, and `cfx test` cli tools or some derivative.  One goal is to reduce the amount of code necessary to implement those tools by moving more work into Firefox.  If that is achieved then I imagine lots of people creating their own customized cfx forks.

Anyhow this patch looks fine to me if it helps with the sim, but it needs tests.

I am curious why the version is not stored in package.json tho.
Here is some reasons:
* when building nightly, the addon version is volatile and is computed in the build script.
-or- in simulator case, the addon version comes from somewhere else. It comes from the b2g version we are targetting, which is defined in an environment variable given to the makefile.
You can see memchaser working around that, to generate nightly builds and hacking install.rdf after cfx xpi is done:
https://github.com/mozilla/memchaser/blob/master/build.xml
* you need to addon version in the build script, for example to name the xpi file name with the addon version in it, or to push it to a URL with the version in it.
It is slitghly easier to sed replace some special vars in a json file than parsing it to fetch the version.
* the version isn't only set in the version field. In the simulator, we put the Firefox OS version in the version field, but also in the name and description.

I'm open for other ways to handle the build of the simulator addon.
See bug 944451 attachment 8343163 [details] [diff] [review] and /b2g/simulator/Makefile.in
I'm mainly trying to prevent hacky build script and uplift necessary improvement to keep it sane.
Comment on attachment 8342510 [details] [diff] [review]
Implements --manifest-overload

Review of attachment 8342510 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine Alex, it needs a test though.
Attachment #8342510 - Flags: review?(evold) → review-
Attached patch Add unit test (obsolete) — Splinter Review
Attachment #8342510 - Attachment is obsolete: true
Assignee: nobody → poirot.alex
Attached patch Add unit test (obsolete) — Splinter Review
Attachment #8344620 - Attachment is obsolete: true
Comment on attachment 8344622 [details] [diff] [review]
Add unit test

I haven't found any explicit test about cfx command line options,
so I hacked something into test_init (which is supposed to test __init__.py) to get that tested.
Attachment #8344622 - Flags: review?(evold)
Comment on attachment 8344622 [details] [diff] [review]
Add unit test

Looks like there are some missing new files here.
Attached patch Add missing fileSplinter Review
One day... I'll stop forgetting new files!
Attachment #8344622 - Attachment is obsolete: true
Attachment #8344622 - Flags: review?(evold)
Attachment #8344847 - Flags: review?(evold)
Comment on attachment 8344847 [details] [diff] [review]
Add missing file

Looks good Alex, Thanks!
Attachment #8344847 - Flags: review?(evold) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.