Closed Bug 1232707 Opened 5 years ago Closed 5 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)

defect

Tracking

(firefox45 fixed, firefox46 fixed)

RESOLVED FIXED
mozilla46
Iteration:
46.1 - Dec 28
Tracking Status
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+
dmose
: 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
Details | Diff | Splinter Review
2.39 MB, patch
Details | Diff | Splinter Review
2.47 MB, patch
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.
Rank: 15
Blocks: 1232720
Blocks: 1233045
Assignee: nobody → standard8
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.
Blocks: 1226706
No longer blocks: 1232720
Depends on: 1231702
This also wants bug 1232720 to land first.
Depends on: 1232720
Rank: 15 → 12
Rank: 12 → 7
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)
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)
Attached patch m-c General renames and moves. (obsolete) — Splinter Review
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)
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 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 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 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 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 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-
Attachment #8707406 - Flags: review?(mh+mozilla) → review+
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...
(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.
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)
Attachment #8707406 - Attachment is obsolete: true
Attachment #8707407 - Attachment is obsolete: true
Attachment #8707409 - Attachment is obsolete: true
(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)
Keywords: leave-open
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.
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 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+
Attachment #8707401 - Flags: review?(edilee)
Depends on: 1239872
In bug 1239872, I'm going to make the jar maker reject your trick for react.js. Please remove this trick.
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 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+
(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.
(or, for that matter, share the file with devtools, since they *also* ship react.js)
btw, devtools use a more recent version /and/ it's not minified.
(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.
(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.
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?
Attachment #8710965 - Attachment is patch: true
Please see previous comments, this includes the follow-up fix for fixing the react layout.
Attachment #8710967 - Flags: approval-mozilla-aurora?
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+
Attachment #8710966 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8710967 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.