Stop using preprocessor in mozscreenshots' bootstrap.js

RESOLVED FIXED in Firefox 58

Status

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: marco, Assigned: mill2540, Mentored)

Tracking

Trunk
mozilla58
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
bootstrap.js is preprocessed only to workaround a build system bug.
Assignee: nobody → mill2540
Mentor: mconley, jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment on attachment 8910049 [details]
Bug 1385233 - Fixed the moz.build so that preprocessor instructions are no longer required in bootstrap.js.

https://reviewboard.mozilla.org/r/181516/#review186822

Looks good. I've pushed your patch to tryserver to verify that mozscreenshots still works with it. The results can be found at https://treeherder.mozilla.org/#/jobs?repo=try&revision=86a20884855baa42653edbb41a77ca67221f6658

Since this change deals with the build system for the screenshots code, we should be able to confirm that nothing broke by seeing that the screenshots jobs complete successfully. After confirmation, I will get this patch landed on our "autoland" repository. After about 6-12 hours the patch should get merged from autoland to mozilla-central.
Attachment #8910049 - Flags: review?(jaws) → review+
Looking at the following link, I don't see any mozscreenshots jobs:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=86a20884855baa42653edbb41a77ca67221f6658&group_state=expanded&filter-tier=1&filter-tier=2&filter-tier=3

I've pushed the parent changeset to our Try server to see what difference your change made. Results for this try push should be available in a few hours: 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d951a4e8de459ad85d3318337ef57523e178510

I don't see anything in this bug that says the particular build issue that this was working around is fixed. Marco, as you filed this bug, do you know that the underlying bug has been fixed?
Flags: needinfo?(mcastelluccio)
Reporter

Comment 4

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Looking at the following link, I don't see any mozscreenshots jobs:

Maybe try using mach try fuzzy? I had a similar problem yesterday where tests wouldn't run (it was for the coverage build though), using mach try fuzzy made them run.

> I don't see anything in this bug that says the particular build issue that
> this was working around is fixed. Marco, as you filed this bug, do you know
> that the underlying bug has been fixed?

I have no idea. I can't see any reference to the build issue in the bug where this file was introduced (bug 1169179). Maybe MattN remembers?
Flags: needinfo?(mcastelluccio) → needinfo?(MattN+bmo)
The problem is exactly what the comment said: the "file doesn't get packaged if not pre-processed.".

To confirm it's fixed, make sure that both unpackaged and packaged (mach package + install) builds build the file and also that a change to that file is also picked up with a `mach build`.

Also note that the current patch isn't fully fixing the bug since install.rdf is still being preprocessed.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8910049 [details]
Bug 1385233 - Fixed the moz.build so that preprocessor instructions are no longer required in bootstrap.js.

https://reviewboard.mozilla.org/r/181516/#review187298

::: browser/tools/mozscreenshots/mozscreenshots/extension/moz.build:18
(Diff revision 1)
>  FINAL_TARGET_PP_FILES += [
> -    'bootstrap.js',
>      'install.rdf',

This is still being pre-processed
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #5)
> Also note that the current patch isn't fully fixing the bug since
> install.rdf is still being preprocessed.

Though it seems like it's doing things the normal way like other in-tree extensions: https://dxr.mozilla.org/mozilla-central/search?q=path%3Ainstall.rdf++%22version%3E%22&redirect=false

It's not clear to me whether that's an issue or not since it's not clear what the main motivator for this bug is.
Reporter

Comment 8

2 years ago
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #5)
> The problem is exactly what the comment said: the "file doesn't get packaged
> if not pre-processed.".

Ok, so we don't know what the underlying issue is, we only know that there is one.

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #7)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #5)
> > Also note that the current patch isn't fully fixing the bug since
> > install.rdf is still being preprocessed.
> 
> Though it seems like it's doing things the normal way like other in-tree
> extensions:
> https://dxr.mozilla.org/mozilla-central/search?q=path%3Ainstall.
> rdf++%22version%3E%22&redirect=false
> 
> It's not clear to me whether that's an issue or not since it's not clear
> what the main motivator for this bug is.

The rdf file doesn't matter, as the reason for this bug is reducing the number of JavaScript files that are preprocessed to speed up the rewriting of coverage reports (bug 1381972).
ok, thanks for clarifying
Summary: Stop using preprocessor in mozscreenshots → Stop using preprocessor in mozscreenshots' bootstrap.js
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #5)
> The problem is exactly what the comment said: the "file doesn't get packaged
> if not pre-processed.".
> 
> To confirm it's fixed, make sure that both unpackaged and packaged (mach
> package + install) builds build the file and also that a change to that file
> is also picked up with a `mach build`.

Robin, can you confirm that your patch is working? Run `mach build && mach package`, then you will find the build packaged as firefox-57.0a1-en-US.win64.zip (my local build is on win64, your filename may differ slightly) in your source-dir/obj-dir/dist/ directory. You can unzip that package and confirm that the bootstrap.js file exists in the mozscreenshots path.

Also, please verify that `mach build installer` generates an installer that includes the file. The installer should be located at source-dir/obj-dir/dist/install/sea/.

You may want to make some simple change to the file to make sure that the packaged and installed file includes your update.
Flags: needinfo?(mill2540)
Assignee

Comment 11

2 years ago
I ran mach package and I don't see the bootstrap.js file, or indeed the browser/tools/ directory at all. However, I also did a vanilla pull from mozilla-central (no patch) and I don't see them either. Does the build tool rename/hide/compress them somewhere? 

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #5)
> > The problem is exactly what the comment said: the "file doesn't get packaged
> > if not pre-processed.".
> > 
> > To confirm it's fixed, make sure that both unpackaged and packaged (mach
> > package + install) builds build the file and also that a change to that file
> > is also picked up with a `mach build`.
> 
> Robin, can you confirm that your patch is working? Run `mach build && mach
> package`, then you will find the build packaged as
> firefox-57.0a1-en-US.win64.zip (my local build is on win64, your filename
> may differ slightly) in your source-dir/obj-dir/dist/ directory. You can
> unzip that package and confirm that the bootstrap.js file exists in the
> mozscreenshots path.
> 
> Also, please verify that `mach build installer` generates an installer that
> includes the file. The installer should be located at
> source-dir/obj-dir/dist/install/sea/.
> 
> You may want to make some simple change to the file to make sure that the
> packaged and installed file includes your update.
Flags: needinfo?(mill2540) → needinfo?(jaws)
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #5)
> The problem is exactly what the comment said: the "file doesn't get packaged
> if not pre-processed.".
> 
> To confirm it's fixed, make sure that both unpackaged and packaged (mach
> package + install) builds build the file and also that a change to that file
> is also picked up with a `mach build`.
> 
> Also note that the current patch isn't fully fixing the bug since
> install.rdf is still being preprocessed.

I can't see where this exists in packaged builds. I do find that bootstrap.js exists in objdir/dist/xpi-stage with and without this patch. I have unzipped both firefox-57.0.en-US.win64.zip and mozharness.zip and can't find any mention of mozscreenshots inside.

I think we should land this patch. Matt, are you OK with that?
Flags: needinfo?(jaws) → needinfo?(MattN+bmo)
It's in the xpi file for the extension I think
Flags: needinfo?(MattN+bmo)
Thanks, I extracted the xpi and the bootstrap.js file is in there and doesn't have the preprocessor comment anymore. I'll land this now.

Comment 15

2 years ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9e6c3c0d2c2
Fixed the moz.build so that preprocessor instructions are no longer required in bootstrap.js. r=jaws
https://hg.mozilla.org/mozilla-central/rev/a9e6c3c0d2c2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.