Closed
Bug 1232707
Opened 7 years ago
Closed 7 years ago
Make a dist directory with code that's exportable to mozilla-central and confirm it'll work
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox45 fixed, firefox46 fixed)
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Whiteboard: [github-addon-ship])
User Story
- Create a new command `make dist` - The command should create a new directory, `dist`. - Within the directory, a `m-c` directory should be created that contains clean output for mozilla-central, to contain: -- The files required to create the add-on -- The generated jsx files -- All test files for the add-on - Create a command, e.g. `make export-m-c` to copy the files across to the relevant places in m-c. - Test the result, with a push to try with coverage for mochitest, xpcshell and marionette
Attachments
(8 files, 3 obsolete files)
39 bytes,
text/x-github-pull-request
|
mikedeboer
:
review+
dmosedale
:
review+
|
Details | Review |
13.88 KB,
patch
|
Details | Diff | Splinter Review | |
4.16 MB,
patch
|
Details | Diff | Splinter Review | |
2.16 MB,
patch
|
Details | Diff | Splinter Review | |
1.62 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
15.91 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.39 MB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.47 MB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
As part of our work, we need to be able to create a clean add-on directory that has just the code we need to export to mozilla-central. See user story for more info.
Assignee | ||
Updated•7 years ago
|
Rank: 15
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → standard8
Assignee | ||
Comment 1•7 years ago
|
||
Having just started to look at this, I think it would be better to depend on landing bug 1231702 first as that's going to be producing better babel output, that we're going to want to pick up here.
Assignee | ||
Updated•7 years ago
|
Rank: 15 → 12
Assignee | ||
Updated•7 years ago
|
Rank: 12 → 7
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8707401 [details] [review] [loop] Standard8:bug-1232707-export > mozilla:master There's a couple of parts to this bug: - This PR sets up the loop repo to create a dist/export-gecko directory, and does some preparation work so that tests will pass in m-c. - The second set of patches are for mozilla-central and are adjusting some files that will remain there, and dealing with moves and the initial imports. Notes: - I've included a work around for l10n at the moment - we use the same file as m-c does now (we currently get away with this as we haven't added strings yet). I'll deal with fixing it in some follow-ups. - This probably isn't perfect, but we need to start getting it landed and the process proved. - We're shipping the generated files for react, but I think that's a good thing, less confusion in m-c. - I've simplified jar.mn. As we're only putting the files we want into m-c, we can use directory matching and save updating a big list all the time (we're also not using the list in the github repo). - There's a separate commit that deals with l10n in the shared tests. I didn't want to export the standalone gaia l10n file, so I swapped the tests to using the desktop l10n file. This required a few changes, including making the desktop l10n api slightly closer to the standalone one. I'm happy to spend time pair reviewing/discussing if anyone wants to.
Attachment #8707401 -
Flags: review?(mdeboer)
Attachment #8707401 -
Flags: review?(edilee)
Attachment #8707401 -
Flags: review?(dmose)
Assignee | ||
Comment 5•7 years ago
|
||
This is the set of changes we need to support the a new (reduced) directory layout for the add-on. glandium: can you review this from the build perspective please? See notes in comment 4 as well.
Attachment #8707406 -
Flags: review?(mh+mozilla)
Attachment #8707406 -
Flags: review?(mdeboer)
Attachment #8707406 -
Flags: review?(edilee)
Attachment #8707406 -
Flags: review?(dmose)
Assignee | ||
Comment 6•7 years ago
|
||
This is just general renames and moves to preseve history a little.
Attachment #8707407 -
Flags: review?(mdeboer)
Attachment #8707407 -
Flags: review?(edilee)
Attachment #8707407 -
Flags: review?(dmose)
Assignee | ||
Comment 7•7 years ago
|
||
This is the diffs generated from a `make git-export` on the branch of the PR. Hence landing the latest code. Try server push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed3d1ab61cb5
Attachment #8707409 -
Flags: review?(mdeboer)
Attachment #8707409 -
Flags: review?(edilee)
Attachment #8707409 -
Flags: review?(dmose)
Comment 8•7 years ago
|
||
Comment on attachment 8707406 [details] [diff] [review] m-c Build changes to support Loop imported as an add-on Review of attachment 8707406 [details] [diff] [review]: ----------------------------------------------------------------- I say: ship it! ::: browser/extensions/loop/README.txt @@ +2,2 @@ > > +Current extension version is: 0.1 I think having the version number here is asking for it to stale. Perhaps it's good to replace this with a notice, like 'Pull requests welcome!'?
Attachment #8707406 -
Flags: review?(mdeboer)
Attachment #8707406 -
Flags: review?(edilee)
Attachment #8707406 -
Flags: review?(dmose)
Attachment #8707406 -
Flags: review+
Comment 9•7 years ago
|
||
Comment on attachment 8707407 [details] [diff] [review] m-c General renames and moves. Review of attachment 8707407 [details] [diff] [review]: ----------------------------------------------------------------- Goodness me. I'm going to trust your brutish abilities with `git mv` and `git rm`! rs=me.
Attachment #8707407 -
Flags: review?(mdeboer)
Attachment #8707407 -
Flags: review?(edilee)
Attachment #8707407 -
Flags: review?(dmose)
Attachment #8707407 -
Flags: review+
Comment 10•7 years ago
|
||
Comment on attachment 8707409 [details] [diff] [review] Import the latest version of the Loop system add-on. Review of attachment 8707409 [details] [diff] [review]: ----------------------------------------------------------------- I started going through all the changes here and realized that it's not really efficient to re-review all these diffs. The main thing here is that all the files need to be in the right place and tests should pass. I guess what I'm saying is: I trust machines more than my own eyes, so if try is happy, I'm happy. ;-) ::: browser/extensions/loop/bootstrap.js @@ +546,5 @@ > + "infobar_button_resume_accesskey" : > + "infobar_button_pause_accesskey"); > + let barLabel = this._getString(this._browserSharePaused ? > + "infobar_screenshare_paused_browser_message" : > + "infobar_screenshare_browser_message2"); OK, I guess you want to make eslint happy, but could you refactor this lot into a (perhaps scoped) function `getNotificationBarStrings(paused)` or something? Or wait, was this added in a another bug?
Attachment #8707409 -
Flags: review?(mdeboer)
Comment 11•7 years ago
|
||
Comment on attachment 8707401 [details] [review] [loop] Standard8:bug-1232707-export > mozilla:master r=me on the Javascript and jar manifest bits. I'd like it if someone else could review the build script and the Python test runner changes. Thanks!
Attachment #8707401 -
Flags: review?(mdeboer) → review+
Comment 12•7 years ago
|
||
Comment on attachment 8707407 [details] [diff] [review] m-c General renames and moves. You're moving files. Please use hg mv to mark them as moved. That should consequently appear in the diff with rename markers and no diff.
Attachment #8707407 -
Flags: review-
Updated•7 years ago
|
Attachment #8707406 -
Flags: review?(mh+mozilla) → review+
Comment 13•7 years ago
|
||
Mark, I'm assuming that you just wanted one of the Hello reviewers, and Mike has already done a fine job. If I'm misunderstanding and you want more eyes, I can do that tomorrow...
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #12) > Comment on attachment 8707407 [details] [diff] [review] > m-c General renames and moves. > > You're moving files. Please use hg mv to mark them as moved. That should > consequently appear in the diff with rename markers and no diff. I've just checked the files listed, and the ones that are being removed in the diff are in fact being completely removed. I did get the title of the patch wrong, it should have been s/moves/deletes/. I did notice however, that for some reason I removed the functional tests directory, which we still want for now. I'll add that back in.
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8707409 [details] [diff] [review] Import the latest version of the Loop system add-on. Cancelling this. I think we'll probably just be able to say r=import or r=<self> in future.
Attachment #8707409 -
Flags: review?(edilee)
Attachment #8707409 -
Flags: review?(dmose)
Assignee | ||
Comment 16•7 years ago
|
||
Updating the patches.
Assignee | ||
Updated•7 years ago
|
Attachment #8707406 -
Attachment is obsolete: true
Attachment #8707407 -
Attachment is obsolete: true
Attachment #8707409 -
Attachment is obsolete: true
Assignee | ||
Comment 17•7 years ago
|
||
Assignee | ||
Comment 18•7 years ago
|
||
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to Dan Mosedale (:dmose) from comment #13) > Mark, I'm assuming that you just wanted one of the Hello reviewers, and Mike > has already done a fine job. If I'm misunderstanding and you want more > eyes, I can do that tomorrow... Dan, if you can review the build/python changes in the PR, that'd be great. If try server passes, I'm going to go ahead with landing the m-c patches. They've all been reviewed and build/python shouldn't affect them too much (worst case we can do a fresh export, which will be much simpler). https://treeherder.mozilla.org/#/jobs?repo=try&revision=064df4b84a6a
Flags: needinfo?(dmose)
Comment 20•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a0d5ef4e35e3 https://hg.mozilla.org/integration/fx-team/rev/e93dcab6dd3d https://hg.mozilla.org/integration/fx-team/rev/556628e979a3
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
I had to back these out in https://hg.mozilla.org/integration/fx-team/rev/03215fabd7fb for breaking ESLint: https://treeherder.mozilla.org/logviewer.html#?job_id=6608007&repo=fx-team
Comment 22•7 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/adbce5cdc592 https://hg.mozilla.org/integration/fx-team/rev/55af065de40e https://hg.mozilla.org/integration/fx-team/rev/18cbd790875f
Assignee | ||
Comment 23•7 years ago
|
||
I repushed with a minor change to .eslintignore to ignore all of Loop's files. Since we're importing some "object" files from github rather than source, I don't think it makes much sense to lint the remainder. We can revisit this later if necessary, but we're primarily not working in m-c so I don't think it'll matter too much.
Comment 24•7 years ago
|
||
OK, I've added some comments to the PR. I don't think any of them are _required_, but spruce up the comments a bit in the Python as noted in the PR comments would be nice for future maintainers.
Flags: needinfo?(dmose)
Comment 25•7 years ago
|
||
Comment on attachment 8707401 [details] [review] [loop] Standard8:bug-1232707-export > mozilla:master Just added a bit of feedback on the build bits too.
Attachment #8707401 -
Flags: review?(dmose) → review+
Assignee | ||
Comment 26•7 years ago
|
||
https://github.com/Standard8/loop/commit/63a0eacccae05cec4c0af2f605b8f05cdd85b00a https://github.com/mozilla/loop/commit/2ce310080e976a3865ec16606911403fc27ba5f6
Keywords: leave-open
Assignee | ||
Updated•7 years ago
|
Attachment #8707401 -
Flags: review?(edilee)
Comment 27•7 years ago
|
||
In bug 1239872, I'm going to make the jar maker reject your trick for react.js. Please remove this trick.
Assignee | ||
Comment 28•7 years ago
|
||
Thanks for the note Mike, this is a bit more explicit than I'd like, but it seems its the only way.
Attachment #8708256 -
Flags: review?(mh+mozilla)
Comment 29•7 years ago
|
||
Comment on attachment 8708256 [details] [diff] [review] Follow to bug 1232707 - Adjust how react is included into Loop in release and debug configurations. Review of attachment 8708256 [details] [diff] [review]: ----------------------------------------------------------------- Thanks
Attachment #8708256 -
Flags: review?(mh+mozilla) → review+
Comment 30•7 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #28) > Created attachment 8708256 [details] [diff] [review] > Follow to bug 1232707 - Adjust how react is included into Loop in release > and debug configurations. > > Thanks for the note Mike, this is a bit more explicit than I'd like, but it > seems its the only way. I'd argue the right way would be to use the same file in both, the non-minified one, and minify the js files during the build, which we do on android and b2g, and could be doing on desktop.
Comment 31•7 years ago
|
||
(or, for that matter, share the file with devtools, since they *also* ship react.js)
Comment 32•7 years ago
|
||
btw, devtools use a more recent version /and/ it's not minified.
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #32) > btw, devtools use a more recent version /and/ it's not minified. For us its not the minification necessarily - the "prod" version is running in their production mode which reduces some of the other code and does less work that's generally only need for debugging. Hence it has better performance overall, which is what we're looking for here. We'll have to think about sharing sometime else. As we're a system add-on now, I think that might be a difficult dependency to handle around upgrade times.
Assignee | ||
Comment 35•7 years ago
|
||
(In reply to Pulsebot from comment #34) > https://hg.mozilla.org/integration/fx-team/rev/b8a038770d4c Also landed in the loop repository at: https://github.com/mozilla/loop/commit/d366ad527d8e4e46edd6c34fcab660c9f67b84c8
Comment 36•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/adbce5cdc592 https://hg.mozilla.org/mozilla-central/rev/55af065de40e https://hg.mozilla.org/mozilla-central/rev/18cbd790875f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 37•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8a038770d4c
Comment 38•7 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #28) > Created attachment 8708256 [details] [diff] [review] > Follow to bug 1232707 - Adjust how react is included into Loop in release > and debug configurations. > > Thanks for the note Mike, this is a bit more explicit than I'd like, but it > seems its the only way. Note that, as OSX universal bustage following this patch landing showed, using the wildcard made us /also/ ship react-prod.js as react-prod.js in all builds. So while this is more explicit, it also removes a file that shouldn't have been there in the first place.
Assignee | ||
Comment 39•7 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: Firefox Hello as a system add-on [User impact if declined]: User's downloading Firefox won't get the same version as is pushed out on the update system. Alternately, each Firefox update will wipe out the system-addon update until it updates again. [Describe test coverage new/current, TreeHerder]: Has been on nightly for a while. [Risks and why]: This is the next update to the Firefox Hello system-addon. It fixes a few issues that are needed for before beta (like disabling the first-time-user tour). This set of patches also re-organise the directory structures to make it simpler for us to import just the add-on code into the repositories. [String/UUID change made/needed]: None. For now, we're still working on the l10n system for Hello, however I've made sure the patch sets don't need L10n changes.
Attachment #8710965 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•7 years ago
|
Attachment #8710965 -
Attachment is patch: true
Assignee | ||
Comment 40•7 years ago
|
||
Please see previous comment.
Attachment #8710966 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 41•7 years ago
|
||
Please see previous comments, this includes the follow-up fix for fixing the react layout.
Attachment #8710967 -
Flags: approval-mozilla-aurora?
Updated•7 years ago
|
status-firefox45:
--- → affected
Comment 42•7 years ago
|
||
Comment on attachment 8710965 [details] [diff] [review] Aurora patch part 1 - Build changes to support imported Loop OK, right before the merge!
Attachment #8710965 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8710966 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8710967 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 43•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/24783f6fbe64 https://hg.mozilla.org/releases/mozilla-aurora/rev/649034372845 https://hg.mozilla.org/releases/mozilla-aurora/rev/97e6ca995311
You need to log in
before you can comment on or make changes to this bug.
Description
•