1232707 - Make a dist directory with code that's exportable to mozilla-central and confirm it'll work

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Mark Banner (: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.

Comment 2

[Mark Banner (:standard8)]

This also wants bug 1232720 to land first.

Comment 3

[GitHub Autolander Bot]

Attachment: [loop] Standard8:bug-1232707-export > mozilla:master

Comment 4

[Mark Banner (:standard8)]

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.

Comment 5

[Mark Banner (:standard8)]

Attachment: m-c Build changes to support Loop imported as an add-on

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.

Comment 6

[Mark Banner (:standard8)]

Attachment: m-c General renames and moves.

This is just general renames and moves to preseve history a little.

Comment 7

[Mark Banner (:standard8)]

Attachment: Import the latest version of the Loop system add-on.

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

Comment 8

[Mike de Boer [:mikedeboer]]

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!'?

Comment 9

[Mike de Boer [:mikedeboer]]

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.

Comment 10

[Mike de Boer [:mikedeboer]]

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?

Comment 11

[Mike de Boer [:mikedeboer]]

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!

Comment 12

[Mike Hommey [:glandium]]

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.

Comment 13

[Dan Mosedale (:dmose, :dmosedale)]

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...

Comment 14

[Mark Banner (:standard8)]

(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 15

[Mark Banner (:standard8)]

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.

Comment 16

[Mark Banner (:standard8)]

Attachment: Build changes to support Loop imported as an add-on. r=mikedeboer,r=glandium

Updating the patches.

Comment 17

[Mark Banner (:standard8)]

Attachment: General renames and deletes preparing for Loop being imported from external repo.

Comment 18

[Mark Banner (:standard8)]

Attachment: Import the latest version of the Loop system add-on. r=Standard8 for import of already reviewed code.

Comment 19

[Mark Banner (:standard8)]

(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

Comment 20

[Pulsebot]

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

Comment 21

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

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

[Pulsebot]

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

Comment 23

[Mark Banner (:standard8)]

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

[Dan Mosedale (:dmose, :dmosedale)]

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.

Comment 25

[Dan Mosedale (:dmose, :dmosedale)]

Comment on attachment 8707401 [details] [review]
[loop] Standard8:bug-1232707-export > mozilla:master

Just added a bit of feedback on the build bits too.

Comment 26

[Mark Banner (:standard8)]

https://github.com/Standard8/loop/commit/63a0eacccae05cec4c0af2f605b8f05cdd85b00a
https://github.com/mozilla/loop/commit/2ce310080e976a3865ec16606911403fc27ba5f6

Comment 27

[Mike Hommey [:glandium]]

In bug 1239872, I'm going to make the jar maker reject your trick for react.js. Please remove this trick.

Comment 28

[Mark Banner (:standard8)]

Attachment: 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.

Comment 29

[Mike Hommey [:glandium]]

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

Comment 30

[Mike Hommey [:glandium]]

(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

[Mike Hommey [:glandium]]

(or, for that matter, share the file with devtools, since they *also* ship react.js)

Comment 32

[Mike Hommey [:glandium]]

btw, devtools use a more recent version /and/ it's not minified.

Comment 33

[Mark Banner (:standard8)]

(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.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b8a038770d4c

Comment 35

[Mark Banner (:standard8)]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/adbce5cdc592
https://hg.mozilla.org/mozilla-central/rev/55af065de40e
https://hg.mozilla.org/mozilla-central/rev/18cbd790875f

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/b8a038770d4c

Comment 38

[Mike Hommey [:glandium]]

(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.

Comment 39

[Mark Banner (:standard8)]

Attachment: Aurora patch part 1 - Build changes to support imported Loop

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.

Comment 40

[Mark Banner (:standard8)]

Attachment: Aurora patch part 2 - General renames and deletes preparing for Loop being imported from external repo.

Please see previous comment.

Comment 41

[Mark Banner (:standard8)]

Attachment: Aurora patch part 3 - Import the latest version of the Loop system add-on.

Please see previous comments, this includes the follow-up fix for fixing the react layout.

Comment 42

[Sylvestre Ledru [:Sylvestre] back june 18th]

Comment on attachment 8710965 [details] [diff] [review]
Aurora patch part 1 - Build changes to support imported Loop

OK, right before the merge!

Comment 43

[Carsten Book [:Tomcat]]

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