1385233 - Stop using preprocessor in mozscreenshots' bootstrap.js

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Marco Castelluccio [:marco]]

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

Comment 1

[Robin Miller]

Attachment: Bug 1385233 - Fixed the moz.build so that preprocessor instructions are no longer required in bootstrap.js.

moz.build had bootstrap.js in FINAL_TARGET_PP_FILES which invokes preprocessing. Solved by moving bootstrap.js to FINAL_TARGET_FILES.

Review commit: https://reviewboard.mozilla.org/r/181516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181516/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 4

[Marco Castelluccio [:marco]]

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

Comment 5

[Matthew N. [:MattN]]

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 6

[Matthew N. [:MattN]]

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

Comment 7

[Matthew N. [:MattN]]

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

Comment 8

[Marco Castelluccio [:marco]]

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

Comment 9

[Matthew N. [:MattN]]

ok, thanks for clarifying

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 11

[Robin Miller]

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.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 13

[Matthew N. [:MattN]]

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

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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