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

RESOLVED FIXED in Firefox 45

Status

defect
P1
normal
Rank:
7
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed, firefox46 fixed)

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 attachments, 3 obsolete attachments)

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
Assignee

Description

4 years ago
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

4 years ago
Rank: 15
Assignee

Updated

4 years ago
Blocks: 1232720
Assignee

Updated

4 years ago
Blocks: 1233045
Assignee

Updated

4 years ago
Assignee: nobody → standard8
Assignee

Comment 1

4 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.
Blocks: 1226706
No longer blocks: 1232720
Depends on: 1231702
Assignee

Comment 2

4 years ago
This also wants bug 1232720 to land first.
Depends on: 1232720
Assignee

Updated

4 years ago
Rank: 15 → 12
Assignee

Updated

4 years ago
Rank: 12 → 7
Assignee

Comment 4

4 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

4 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

4 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

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

Comment 14

4 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

4 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

Updated

4 years ago
Attachment #8707406 - Attachment is obsolete: true
Attachment #8707407 - Attachment is obsolete: true
Attachment #8707409 - Attachment is obsolete: true
Assignee

Comment 19

4 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)
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Comment 23

4 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.
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+
Assignee

Updated

4 years ago
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.
Assignee

Comment 28

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

Comment 33

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

3 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

3 years ago
Attachment #8710965 - Attachment is patch: true
Assignee

Comment 41

3 years ago
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.