bootstrap.js is preprocessed only to workaround a build system bug.

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.

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?

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

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.

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.

(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

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

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.

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

It's in the xpi file for the extension I think

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.

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