Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reproducible Build #306

Closed
lidel opened this issue Nov 13, 2017 · 15 comments
Closed

Reproducible Build #306

lidel opened this issue Nov 13, 2017 · 15 comments
Assignees
Labels
kind/maintenance Work required to avoid breaking changes or harm to project's status quo P0 Critical: Tackled by core team ASAP topic/security Work related to security
Milestone

Comments

@lidel
Copy link
Member

lidel commented Nov 13, 2017

Post-submission review process at Mozilla kicked in for ipfs-companion and requires us to provide source code + build steps that will produce exactly the same code as one published to NPM:

The following information is needed to complete your review:

  1. This version contains obfuscated, minified, concatenated or otherwise machine-generated code. Please provide the original sources, together with instructions on how to generate the final XPI. Source code must be provided as an archive and uploaded using the source code upload field, which can be done during submission or on the version page in the developer hub.

Please read through the instructions at https://developer.mozilla.org/en-US/Add-ons/Source_Code_Submission. If sources have not been provided within 7 days, this version will be rejected and removed from our queues.
More Information: https://blog.mozilla.org/addons/2016/04/05/improved-review-time-with-links-to-sources/

Established libraries (eg: JQuery, bootstrap) do not require the inclusion of their sources.
Please use non-custom versions of jquery-ui, as well as the stock version of jquery and others, that match the checksums in
https://github.com/mozilla/amo-validator/blob/master/validator/testcases/hashes-allowed.txt

Problem 1
I am unable to replicate build results for two of four dependencies: ipfs-api and is-ipfs.

I downloaded https://github.com/ipfs/js-ipfs-api/releases/tag/v15.0.1 but was unable to produce the same file as https://unpkg.com/ipfs-api@15.0.1/dist/index.min.js -- after running npm install && npm run build minified code looks different. Same goes for is-ipfs.

This may be due to different version of node, npm or lack of package-lock.json / yarn.lock.
Mozilla Reviewer is okay with using specific versions, as long as we provide step-by-step build instruction that result in correct file, matching the one from NPM.

Problem 2
We have 7 days to provide "sources and build steps", otherwise ipfs-companion will be removed.

@diasdavid @dignifiedquire @fbaiodias – any ideas on how to address this?

@lidel lidel added kind/maintenance Work required to avoid breaking changes or harm to project's status quo P0 Critical: Tackled by core team ASAP topic/security Work related to security labels Nov 13, 2017
@dignifiedquire
Copy link
Member

Created a test branch: ipfs-inactive/js-ipfs-http-client#626 please test

@olizilla
Copy link
Member

@lidel would be be allowed to submit a new version of the add-on that was built with a package-lock.json file? That way the transitive dependencies would all be pinned at specific versions as well as the direct ones. That plus a Dockerfile should be enough to get a reproducible build for future releases.

I'll submit a PR with a Dockerfile and a package-lock.json

@olizilla
Copy link
Member

Ok so, i'm pretty sure that we're ok on this.

From the docs Example_Desired_outcome section of the code submission docs, it sounds like they want to be able to run the same commands we did to create the release, and be able to diff the output with the contents of the uploaded bundle.

i've tried running the build in docker (simulating debian jessie), and running it locally on osx, and recursively diff-ing the unzipped build artefacts reveals no differences. 🎉 I think that's all that moz want to see.

Looking at the source for a popular firefox addon, emoji-helper it looks like they are doing nothing fancier than adding a packge-lock.json. I think we should do that too, but I don't think we need to do anything urgently to get through the review process, unless moz have specific questions.

@olizilla
Copy link
Member

@lidel i've just re-read the title of this issue... did moz already raise an issue about

provide source code + build steps that will produce exactly the same code as one published to NPM.

@lidel
Copy link
Member Author

lidel commented Nov 16, 2017

@olizilla

would be be allowed to submit a new version of the add-on that was built with a package-lock.json file?

Yes, we need to make new releases (extension + deps: js-ipfs-api and is-ipfs), as we are unable to replicate build of already submitted one.
One thing: we had mixed experiences with package-lock.json (development-wise).
I think yarn.lock and your build steps from ipfs-inactive/js-ipfs-http-client#626 (comment) may be faster/better/safer.
Are there any cons to it?

did moz already raise an issue about (..)

Indeed, they already requested that (see #306 (comment), added original feedback there)

@lidel
Copy link
Member Author

lidel commented Nov 16, 2017

Steps are:

  1. make reproducible releases of js-ipfs-api and is-ipfs -- see [WIP] Reproducible builds ipfs-inactive/js-ipfs-http-client#626 (comment)
  2. switch ipfs-companion to new deps and make a new release (@lidel)
  3. submit release to Mozilla with build steps from [WIP] Reproducible builds ipfs-inactive/js-ipfs-http-client#626 (comment) + a note that on some platforms there is one line difference that can be eliminated by building in docker ([WIP] Reproducible builds ipfs-inactive/js-ipfs-http-client#626 (comment)) -- I hope this will be enough to push this version through the review. (@lidel)

So we are kinda waiting for 1)

PS. I wonder if it would not be easier to browserify entire thing (#311) instead 🤔

@olizilla
Copy link
Member

PS. I wonder if it would not be easier to browserify entire thing (#311) instead 🤔

Strongly agree! browerify + a yarn.lock means the add-on bundle will be reproducible. The package.json in js-ipfs-api points to it's src/main.js as it's main, so browserify will bundle that rather than the index.min.js that we can't recreate.

It's still worth getting those libs builds to be reproducible, but it'd be nice to get that off the critical path to getting moz happy with ipfs-companion.

One thing: we had mixed experiences with package-lock.json (development-wise).
I think yarn.lock and your build steps from ipfs-inactive/js-ipfs-http-client#626 (comment) may be faster/better/safer.
Are there any cons to it?

I've not used yarn enough to say. I've not had any problems with npm, but I know that the package-lock merging dev experience needs improving. I saw this nugget from kat the other day

That file isn't really meant for human consumption. I've seen some projects mark it as a binary file which will do all the good you need.

To fix conflicts: git co package-lock.json --theirs && npm i then add and --continue.

https://twitter.com/maybekatz/status/930523154253877248

which I'm looking forward to trying.

@lidel
Copy link
Member Author

lidel commented Nov 17, 2017

Perhaps lock merging with --theirs could be run automatically run via postmerge hook (package.json)?
Are there any edge cases that would be broken with such automation?

@lidel
Copy link
Member Author

lidel commented Nov 19, 2017

Status update: 7 day window from Mozilla will end today and we need to solve it either by enabling yarn+docker in deps (ipfs-inactive/js-ipfs-http-client#626) or switching extension's build to browserify (#311).
I don't think extension will be disabled for existing users, but it won't be possible to install it by new ones.

(Worst case scenario is an emergency release with browserify that does not have working tests.)

@olizilla
Copy link
Member

@lidel I know @alanshaw has made good progress on fixing the tests on the browserify pr. It'll be mergable today.

@lidel lidel changed the title Reproducible Build of NPM Artifacts: ipfs-api, is-ipfs Reproducible Build Nov 20, 2017
lidel added a commit that referenced this issue Nov 20, 2017
Removes private fields from bundled package.json, part of #306 effort
lidel added a commit that referenced this issue Nov 20, 2017
Adds `npm run docker-build` command, part of #306  effort
@lidel
Copy link
Member Author

lidel commented Nov 21, 2017

Alright, we now have two ways of making a reproducible build of build/ipfs_companion-*.zip:

With Nave+Yarn

npm install -g nave
nave use 8.9.1
npm run yarn-build

With Docker+Yarn:

npm run docker-build

I feel quite comfortable with this setup (big thanks to @olizilla for pushing this!).
There is still one line diff (caused by js-ipfs-api: ipfs-inactive/js-ipfs-http-client#626 (comment)) but it should not be a problem, and will be fixed upstream eventually.

Let's keep this issue open until first "reproducible release" is reviewed and accepted at Mozilla.

@lidel
Copy link
Member Author

lidel commented Nov 26, 2017

I've just submitted v2.1.0, fingers crossed 🤞

@lidel lidel added this to the 2018-Q1 milestone Dec 2, 2017
@daviddias
Copy link
Member

image

This is done ✅, right? Are we missing something?

@daviddias
Copy link
Member

Some really cool stats!! :D

image

@daviddias daviddias assigned olizilla and unassigned dignifiedquire and fbaiodias Dec 4, 2017
@lidel
Copy link
Member Author

lidel commented Dec 4, 2017

This is done ✅, right? Are we missing something?

I think it is done for now, yarn-based release build seems to not raise any red flags
(sadly there is no indicator at the AMO site, but it's been a week, so.. 🙃 ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/maintenance Work required to avoid breaking changes or harm to project's status quo P0 Critical: Tackled by core team ASAP topic/security Work related to security
Projects
None yet
Development

No branches or pull requests

5 participants