Alexandre Poirot [:ochameau] Assignee
	Description • 6 years ago

Debugger update v51 (bug 1460371) made the debugger.html ship individual modules instead of a single bundle.
But they are still compiled Javascript requiring a build step. JSX, flow syntax and esmodules require some processing.
Because of that, the original sources are still only on GitHub. But to transpile the modules, we only need to run Babel with a fixed set of plugins.
And to run Babel, we only need NodeJS, but not npm/yarn/node_modules/webpack.
We only need to call node, with a custom build script calling Babel. Babel being vendored into mozilla-central.

So if we can get to run Node at build time, we could start having sources in mozilla-central instead of the transpiled version of the modules!

I prototyped that here:
* m-c branch: https://github.com/ochameau/mozilla-central/tree/mc-modules
* github branch: https://github.com/ochameau/debugger.html/commits/mc-modules

The interesting bits are:
* a "NodeModule" template function exposed to moz.build files:
https://github.com/ochameau/mozilla-central/blob/7150f76f583903fef2ebd50c61455ad91373a902/devtools/client/debugger/new/node-templates.mozbuild#L8-L28
This function helps specifying which files should be transpiled in moz.build files.
It ends up calling "copy-module.js" nodejs script with the source file to be transpiled as well as the objdir path for the transpiled version passed as arguments.
def NodeModules(*modules):
    base = FINAL_TARGET_FILES.chrome.devtools.modules
    for dir in RELATIVEDIR.split('/'):
        base = base[dir]

    for m in modules:
        GENERATED_FILES += [m]

        bundle = GENERATED_FILES[m]
        bundle.script = '/devtools/client/debugger/new/node.py:generate'
        bundle.inputs = ['/devtools/client/debugger/new/build/copy-module.js', m]
        bundle.flags = ['!' + m]

        dest = base
        for s in m.split('/')[:-1]:
            dest = dest[s]
        dest += ["!" + m]

* The python script called by the NodeModules function
https://github.com/ochameau/mozilla-central/blob/7150f76f583903fef2ebd50c61455ad91373a902/devtools/client/debugger/new/node.py#L7-L10
This is just a wrapper to call NodeJS.

def generate(output, node_script, src, dst):
  cmd = ["node", node_script, src, os.path.abspath(dst)]
  os.chdir(os.path.dirname(node_script))
subprocess.check_call(cmd)

Ideally, we could "daemonize" this node script from here to prevent spawing a new node process for each file.

* The build script evaluated in NodeJS to transpile Debugger sources into DevTools Modules that can be loaded via DevTools module loader:
https://github.com/ochameau/mozilla-central/blob/7150f76f583903fef2ebd50c61455ad91373a902/devtools/client/debugger/new/build/copy-module.js
This module only uses built-in modules of node and Babel.
Babel itself is landed into mozilla-central as a one file:
  https://github.com/ochameau/mozilla-central/blob/7150f76f583903fef2ebd50c61455ad91373a902/devtools/client/debugger/new/build/babel.js

* Finally, shipping a JSX files from m-c is as simple as that:
https://github.com/ochameau/mozilla-central/blob/15142334aab264d62526280f638c3572805765d1/devtools/client/debugger/new/src/actions/moz.build
NodeModules(
    'ast.js',
    'breakpoints.js',
    'coverage.js',
    ...
)

	Alexandre Poirot [:ochameau] Assignee
	Comment 1 • 6 years ago

Here is the main patch of my WIP.

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 6 years ago

Comment on attachment 8975849 [details] [diff] [review]
mach build for debugger modules

This is the patch we just discussed in the GitHub meeting.

This is still a work in progress as there is some errors (circular deps warnings) and a performance issues (execute node for each transpiled file).
This patch is also very specific to the debugger and I imagine we could make it slightly more generic.

But I would be interested in overall feedback on the goal here:
  Use nodejs to process individual files without involving npm/yarn/node_modules/webpack. Just nodejs, a custom build script and a copy of babel.

With this approach we don't get into the issues around:
* maintaining copies of node_modules/having to interact with npm
* introducing yet another build tool and having to expose its internal dependency table to mach/mozilla-central build system.

The core of this patch is in /devtools/client/debugger/new/node-templates.mozbuild. See comment 0 for more info about this patch.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 3 • 6 years ago

Comment on attachment 8975849 [details] [diff] [review]
mach build for debugger modules

Review of attachment 8975849 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/debugger/new/node.py
@@ +4,5 @@
> +import os;
> +import subprocess
> +
> +def generate(output, node_script, src, dst):
> +  cmd = ["node", node_script, src, os.path.abspath(dst)]

I built support for finding Node.js at configure time, exposing it to the build system, etc; and I built most of the support for Node-producing toolchain jobs.  It will not be difficult to base this on top of that works so that the Node invocations are inline with build system conventions.

That work is at the base of https://treeherder.mozilla.org/#/jobs?repo=try&revision=d138f854139f2e389867b01d2f2afe59f2975783.

	Alexandre Poirot [:ochameau] Assignee
	Comment 4 • 6 years ago

(In reply to Nick Alexander :nalexander from comment #3)
> I built support for finding Node.js at configure time, exposing it to the
> build system, etc; and I built most of the support for Node-producing
> toolchain jobs.  It will not be difficult to base this on top of that works
> so that the Node invocations are inline with build system conventions.
> 
> That work is at the base of
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d138f854139f2e389867b01d2f2afe59f2975783.

I would be happy to rebase on top of this work. Is there bugs opened to track that?
I imagine all these patches would have to land first.

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 6 years ago

Comment on attachment 8975849 [details] [diff] [review]
mach build for debugger modules

Sorry, I did not intend to ask for review, but feedback instead.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 6 • 6 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > I built support for finding Node.js at configure time, exposing it to the
> > build system, etc; and I built most of the support for Node-producing
> > toolchain jobs.  It will not be difficult to base this on top of that works
> > so that the Node invocations are inline with build system conventions.
> > 
> > That work is at the base of
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d138f854139f2e389867b01d2f2afe59f2975783.
> 
> I would be happy to rebase on top of this work. Is there bugs opened to
> track that?
> I imagine all these patches would have to land first.

No, I mean that we want to put your work on top of the following commits:

7d2ee493a46cNA Bug NODE - Add opt-in Node environment linux64 build job (opt-node-environment).
ac3e841038e4NA Bug NODE - Part 2: Add node toolchain tasks for linux-x64 and win-x64.
c72996742b26NA Bug NODE - Part 1: Add NODE and NPM programs to configure; add --enable-node-environment.
83223e4e543fNA Bug NODE - Pre: Correct what looks like a typo in screenshots.

and do some rewriting to get things into shape for configure and the build system.  In particular, we should:

1) make the optional Node environment into a required environment
2) remove the NPM bits
3) include the Node toolchains in all jobs
4) use config.substs['NODE'] in your node.py

I will try to take a look at doing this.  I'm really just trying to head off an obvious problem, which is that we're taking "node" from the PATH.  I want gps to look past that, since it's mostly solved already.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 7 • 6 years ago

I've continued evolving Nick's patches; the current WIP is at https://github.com/mozilla/gecko/compare/central...dmose:node-proto

The yarn/node_manager removal in particular is already done; I need to look at the commits a bit more to see the best way to extract them.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 8 • 6 years ago

I've rebased my Node work against latest mozilla-central, and squashed/reorged a fair number of commits so that it's easier to figure out what's going on in pieces of it, and it should be easier to understand and various chunks of it: 

https://github.com/mozilla/gecko/compare/central...dmose:node-proto

In particular, the first commits are all of the node-related changes (still WIP, but functional).  I think it might make sense for :apoirot to base his work on those commits; I'd love feedback from :apoirot and :nalexander about that:

https://github.com/dmose/gecko/compare/fde11f6%5E...abf8217%5E?expand=1

Note that the two main advantages over the commits that nick suggested is that these are rebased against the current tip, and the npm and yarn stuff has been excised.  I believe that tasks 1, 3, and 4 from comment 6 still need to be done.

Another subchunk is the activity-stream changes to use the above node commits, which may be of interest to folks here looking at the node-related chunks to get some more context:

https://github.com/dmose/gecko/compare/10a7804...f146ae0?expand=1

Finally, I've pushed this up to try (without running any jobs) just so that it's available on hg for folks who are primarily working there:

https://hg.mozilla.org/try/pushloghtml?changeset=25b3a2aed2bfc778b71ad522f4d1944d2c848f6a

	Dan Mosedale (:dmosedale, :dmose)
	Comment 9 • 6 years ago

Upon looking at my node changes, I see that part (or maybe all) of tools/node/mach_commands.py could also be excised.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 10 • 6 years ago

Nick, Alexander, if you guys think moving forward with node patches I've linked to is the right way to base the work in this bug on, I'm happy to split them out into a separate bug and help them move forward more quickly.  Let me know...

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 11 • 6 years ago

(In reply to Dan Mosedale (:dmose) from comment #10)
> Nick, Alexander, if you guys think moving forward with node patches I've
> linked to is the right way to base the work in this bug on, I'm happy to
> split them out into a separate bug and help them move forward more quickly. 
> Let me know...

Sorry for the delayed reply.  Yes, I think one of you or Alexandre should purge even more of the tree you've posted (e.g., drop --enable-node-environment, add Windows builds, test in all build types) and then Alexandre should rebase on top of that work and use `CONFIG['node']` in his scripts.  I was hoping to have a look at this but time is very tight.

	Gregory Szorc [:gps]
	Comment 12 • 6 years ago

Comment on attachment 8975849 [details] [diff] [review]
mach build for debugger modules

Review of attachment 8975849 [details] [diff] [review]:
-----------------------------------------------------------------

From a build system perspective, the approach of this patch is fine. Using GENERATED_FILES is our standard mechanism for declaring custom build actions.

We will need to use the node binary discovered by configure rather than hardcoding `node`. This should be a separate patch. And support for detecting Node in configure should ideally land separately from this. So best to split into its own bug.

One concerning part about this is the comment about jar.mn processing. If a file is listed in jar.mn, it *must* go through the "mozpack" layer used for processing jar.mn files. This is because of various packaging requirements. It's been a while since I've looked at the code, but I /think/ GENERATED_FILES automagically integrates with the jar.mn processing code. So I /think/ we should be fine.

	Alexandre Poirot [:ochameau] Assignee
	Comment 13 • 6 years ago

Comment on attachment 8975849 [details] [diff] [review]
mach build for debugger modules

Michael, Chris, I'm would like your feedback on this patch that allows to use node to build devtools debugger source directly during `./mach build` (for now, we land the "generated" sources in mozilla-central, but we would like to land the orignal one and let m-c build system automatically compile JSX to JS).

The challenge of this patch is about how to call a node process to preprocess the source files. I think `GENERATED_FILES` is a quite natural option to do such thing. See :gps feedback comment 12 and comment 0 for an overall description of the patch.

But this patch is not satisfying as-is. Because it is very slow.
We end up executing node for each debugger source files, and there is many of them. It highlights how slow node can be to startup.
We do want to execute node only once. I had in mind to run it as a sort of daemon that can be reused multiple time during one build, but :gps discourages me from doing that as it doesn't match build system constraints.
Instead, he suggested to ping you to see if we can somehow make it so that we can call node once for all the sources files to build.
But to be honest, I have no idea if that is something we can achieve via `GENERATED_FIELS` as-is, nor how we could tweak it to do that...

So your feedback/help would be much appreciated here!

	Chris Manchester (limited bugmail, email directly)
	Comment 14 • 6 years ago

Comment on attachment 8975849 [details] [diff] [review]
mach build for debugger modules

Review of attachment 8975849 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/debugger/new/node-templates.mozbuild
@@ +14,5 @@
> +    for dir in RELATIVEDIR.split('/'):
> +        base = base[dir]
> +
> +    for m in modules:
> +        GENERATED_FILES += [m]

This is an example of the syntax we have for multiple outputs in `GENERATED_FILES`, which may get us part of the way here: https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/accessible/xpcom/moz.build#57

	Michael Shal [:mshal]
	Comment 15 • 6 years ago

Comment on attachment 8975849 [details] [diff] [review]
mach build for debugger modules

We discussed this in person at the all hands. :ochameau will rebase this patchset against :dmose's latest node.py action, and then we will help fix the circular dependency error messages and improve the speed by combining node invocations. Can you re-request feedback when that is ready for us to take a look?

	Dan Mosedale (:dmosedale, :dmose)
	Comment 16 • 6 years ago

:ochameau, here is a try build with my current patchset, rebased against this morning's (Pacific time) mozilla-central: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebc6f48d395c2815bc7ec509395545b5427e9119

If you prefer to work against git, this branch currently contains the same code as that try push:

https://github.com/dmose/gecko/compare/central...dmose:node-only?expand=1

	Alexandre Poirot [:ochameau] Assignee
	Comment 17 • 6 years ago

Here is an updated patch queue.
Sorry, but mozreview, nor git bz attach work for these patches. I don't get why, but both stay pending and never complete...
Pushing to github takes too much time as it want to upload the whole repo for some reason (>1GB!)
I tried phabricator, but nothing works with cinnabar...

So here is the patches from a try push:
* https://hg.mozilla.org/try/rev/ed32113e85203213cf72676127714be1f9535c39
  Land debugger sources in m-c
* https://hg.mozilla.org/try/rev/b60fdc1c6cc11e452a312ce6aa72548320bb4ef0
  Implements "NodeModules" template and use it for building the debugger

I rebased against :dmose work. Without reusing his node.py script yet as I'm focusing on the performance issue and multiple inputs/outputs for now. But I'm now using `buildconfig.substs['NODE']` which allows to find the right nodejs path.
With that I got a green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff7e9ef8e3edc528db61dc4a576940b68d912082

But bug 1454912 broke this patch.
I had to workaround it by reverting the use of .stub files.
See the modification made to `python/mozbuild/mozbuild/backend/recursivemake.py` in the second changeset
If I don't do that, I get errors like this:
  0:29.33 ast.js.stub
  0:30.15 touch: cannot touch '.deps/main.js.stub': No such file or directory

Which I was not able to address.
Even using the file descriptor passed as first argument doesn't address this error :/
But may be the new .stub feature is incompatible with multiple inputs/outputs approach?
I would expect the build system to provide paths to .deps/file.stub, but I was not able to get these paths in any possible way. The given file descriptor isn't targeting .stub file either. Does the python script has to ignore the FD and compute the right path to the stub file itself?

Otherwise, this patch now runs nodejs once per folder.
On my VM, `./mach build` after a clobber runs in 1m22 with this patch.
But it completes in 29s without it, so its impact is still very significant!
With the previous patch, running it once per file, the build took 3m30 to complete...

I'm still getting warnings like this, but it doesn't affect the build result:
  circular dependencies:
  make[5]: Circular index.js.pp <- /mnt/desktop/gecko-dev/devtools/client/debugger/new/src/workers/pretty-print/index.js dependency dropped.
  index.js.pp
What may confuse the build system is that the source and the generated files are having the same file name.
Whereas all the usages I saw in m-c, the input and output files were having distinct names.
For example the input would have a .in suffix and the output won't:
  https://searchfox.org/mozilla-central/source/build/moz.build#80-84
Or the input is one type of file like .conf/.py and the ouput another type like .h/.cpp:
  https://searchfox.org/mozilla-central/source/accessible/xpcom/moz.build#55-59

Finally, it may be related but I see a lot of files created here:
  devtools/client/debugger/new/build/*.js
and also here:
  devtools/client/debugger/new/build/.deps/*.pp
It is surprising as it is only a subset of all modules. It looks like it creates one file per folder.

	Alexandre Poirot [:ochameau] Assignee
	Comment 18 • 6 years ago

I was finally able to push the patches to phabricator, you can find them here:
  https://phabricator.services.mozilla.com/D1741

	Michael Shal [:mshal]
	Comment 19 • 6 years ago

Feedback posted in phabricator! The files created in the srcdir appears to be because of the chdir in the node.py script. I think that line can just be deleted, since we want to run the script and create files in the objdir.

	Alexandre Poirot [:ochameau] Assignee
	Comment 20 • 6 years ago

I pushed another revision with your comments addressed, but I now get empty files being generated.
Any idea why?

	Michael Shal [:mshal]
	Comment 21 • 6 years ago

I suspect this is because the GENERATED_FILES treats the first output specially, so the python wrapper might be creating that file for you when you don't want it to in this case. However, I'm having trouble pulling in the patches from phabricator - what branch or tree are your patches based on? For some reason when I try to import, it can't find the base commit and then fails to apply the patches locally due to conflicts. If you have a try push with the new patches I can try pulling from there instead.

	Alexandre Poirot [:ochameau] Assignee
	Comment 22 • 6 years ago

(In reply to Michael Shal [:mshal] from comment #21)
> I suspect this is because the GENERATED_FILES treats the first output
> specially, so the python wrapper might be creating that file for you when
> you don't want it to in this case. However, I'm having trouble pulling in
> the patches from phabricator - what branch or tree are your patches based
> on? For some reason when I try to import, it can't find the base commit and
> then fails to apply the patches locally due to conflicts. If you have a try
> push with the new patches I can try pulling from there instead.

Here is a try push, after rebasing on today's m-c:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56ddcbf605831e1ac9a32981804a3c5aaf3dc09

And yes, that was an issue with the python code.
When I do similar workaround than here, everything works again:
  https://searchfox.org/mozilla-central/source/mobile/android/base/moz.build#180-184
Hopefully, we can avoid such workaround here...

So here is the issues I'm still having with the last patch:
* this workaround
* still some cyclic warnings like:
  make[5]: Circular .deps/moz.build.stub <- /mnt/desktop/gecko-dev/devtools/client/debugger/new/src/actions/breakpoints.js dependency dropped.
* build time still looks too slow when building once per folder.
  Today, I build mozilla-central in 36s while it takes 1m41s with this patch.
  Any idea on how we could execute node only once?
  Should I do only one NodeModules() in a root folder with all the files?
* merge with Dan's node.py once we know everything is good

	Michael Shal [:mshal]
	Comment 23 • 6 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #22)
> So here is the issues I'm still having with the last patch:
> * this workaround

I think we can clean up the workaround slightly by just listing the stub file in the outputs, and avoid passing it as an input to the script. Eg:

diff --git a/devtools/client/debugger/new/node-templates.mozbuild b/devtools/client/debugger/new/node-templates.mozbuild
index f97040d1f6a1e..6fdb2719469b9 100644
--- a/devtools/client/debugger/new/node-templates.mozbuild
+++ b/devtools/client/debugger/new/node-templates.mozbuild
@@ -25,9 +25,8 @@ def NodeModules(*modules):
 
     # For the same reason than https://searchfox.org/mozilla-central/source/mobile/android/base/moz.build#180-184
     # we have to insert a first entry as recursivemake overrides the first entry and we end up with empty files
-    # for the first file only. This file has to exists, so take moz.build as we know it exists in every directories.
-    modules = ("moz.build",) + modules
-    outputs = tuple(modules)
+    # for the first file only.
+    outputs = tuple(("node.stub",) + modules)
     GENERATED_FILES += [outputs]
 
     bundle = GENERATED_FILES[outputs]
diff --git a/devtools/client/debugger/new/node.py b/devtools/client/debugger/new/node.py
index a3b97aad68ce4..50703750fd6d3 100644
--- a/devtools/client/debugger/new/node.py
+++ b/devtools/client/debugger/new/node.py
@@ -6,7 +6,6 @@ import subprocess
 def generate(output, node_script, *modules):
   node = buildconfig.substs['NODE'];
   cmd = [node, node_script]
-  # Remove the first item of `modules`, that is a workaround, see node-templates.mozbuild.
-  cmd.extend(modules[1:])
+  cmd.extend(modules)
 
   subprocess.check_call(cmd)


Here, I've changed the name of the dummy file to "node.stub" rather than "moz.build" to avoid confusion, and since it's not listed in the inputs, we no longer need it to be an existing file. Since it isn't listed as an input, we don't have to skip the first input in node.py

I filed bug 1471995 so that we don't need to have the extra file in the outputs section, but hopefully this is now a small enough workaround that we don't need to block this bug on having that fixed.

> * still some cyclic warnings like:
>   make[5]: Circular .deps/moz.build.stub <-
> /mnt/desktop/gecko-dev/devtools/client/debugger/new/src/actions/breakpoints.
> js dependency dropped.

I see this in your try push, but for some reason I don't see it when building locally. Do you see this on your local machine as well? I'm still trying to reproduce it.

> * build time still looks too slow when building once per folder.
>   Today, I build mozilla-central in 36s while it takes 1m41s with this patch.
>   Any idea on how we could execute node only once?
>   Should I do only one NodeModules() in a root folder with all the files?

Are there additional patches that create more node invocations than what you have in https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56ddcbf605831e1ac9a32981804a3c5aaf3dc09 ?

When I ran this locally, there are ~34 node invocations, and it takes only about 8 seconds to run all of them on my laptop. The incremental build time for building a single directory is under a second. Personally, I think that is really quite reasonable and not worth further optimization. But if you have further patches that bump up the number of invocations significantly, we can look into doing fewer but larger NodeModules() invocations.

> * merge with Dan's node.py once we know everything is good

One thing we will need to address is how to communicate the input files read from the node invocation back to the python process so that we can track dependencies correctly. Right now, if you were to change eg: devtools/client/debugger/new/src/utils/breakpoint/astBreakpointLocation.js, make does not re-invoke node.py:copy-module.js. We can fix this in your node.py by doing something like:

  subprocess.check_call(cmd)
+  return set(modules)

But that will only account for the inputs that we are listing in the moz.build file, not any additional files that copy-module.js (or whatever the underlying javascript file is) may open.

	Alexandre Poirot [:ochameau] Assignee
	Comment 24 • 6 years ago

(In reply to Michael Shal [:mshal] from comment #23)
> (In reply to Alexandre Poirot [:ochameau] from comment #22)
> > So here is the issues I'm still having with the last patch:
> > * this workaround
> 
> I think we can clean up the workaround slightly by just listing the stub
> file in the outputs, and avoid passing it as an input to the script. Eg:

Thanks, I included that in the last revision.

> > * still some cyclic warnings like:
> >   make[5]: Circular .deps/moz.build.stub <-
> > /mnt/desktop/gecko-dev/devtools/client/debugger/new/src/actions/breakpoints.
> > js dependency dropped.
> 
> I see this in your try push, but for some reason I don't see it when
> building locally. Do you see this on your local machine as well? I'm still
> trying to reproduce it.

Yes I still reproduce locally.
Here is after a clobber, on an artifact build:
 0:29.48 make[5]: Circular .deps/node.stub.stub <- /mnt/desktop/gecko-dev/devtools/client/debugger/new/src/main.js dependency dropped.
 0:29.48 node.stub.stub
It doesn't log such warning on incremental builds, only on first ./mach build run.
When I touch `devtools/client/debugger/new/src/actions/ast.js`, it only prints:
  0:05.91 node.stub.stub
  (But 3 times, see my next comment)

> > * build time still looks too slow when building once per folder.
> >   Today, I build mozilla-central in 36s while it takes 1m41s with this patch.
> >   Any idea on how we could execute node only once?
> >   Should I do only one NodeModules() in a root folder with all the files?
> 
> Are there additional patches that create more node invocations than what you
> have in
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c56ddcbf605831e1ac9a32981804a3c5aaf3dc09 ?

No, all have been added in this changeset:
  https://hg.mozilla.org/try/rev/1467f6ac74387a8b3317b606d9918c0cdee38f5e
This is the patch introducing the 34 NodeModules() definitions in moz.build.
I'm not expecting any increase for the debugger.
Now, the current focus is the debugger, but I can easily see some other part of devtools,
like reps wanting to use that new mechanism in followups.

> When I ran this locally, there are ~34 node invocations, and it takes only
> about 8 seconds to run all of them on my laptop. The incremental build time
> for building a single directory is under a second. Personally, I think that
> is really quite reasonable and not worth further optimization. But if you
> have further patches that bump up the number of invocations significantly,
> we can look into doing fewer but larger NodeModules() invocations.

Did you compared without these patches?
For me, it increased the artifact build time by more than 2x,
I don't find it reasonable to only build the debugger.
If other projects start using this, this will explode artifact build time.

I looked more closely at what we were running and get a very different result.
I see 86 runs of nodejs (88 times on the revision you reviewed, 86 on the new one I submit today),
the first call is `nodejs --version`, the rest is calling copy-module.js.
It looks like, when there is more than one module per folder, node.py is called multiple times with the same arguments (i.e. all modules for the folder). It seems to call node.py 3 times per folder.

I can't make any sense of this. I added print calls to NodeModules and it seems to be called the right number of times,
but somehow the population of GENERATED_FILES ends up generating duplicated calls to node.py.
The pattern seems to be:
 * One call to node.py if there is only one file passed to NodeModules()
 * Three calls to node.py if there is more than one file

See in attachment the result of:
  strace -f -e trace=execve -s 1024 ./mach build 2>&1 | grep nodejs

> > * merge with Dan's node.py once we know everything is good
> 
> One thing we will need to address is how to communicate the input files read
> from the node invocation back to the python process so that we can track
> dependencies correctly. Right now, if you were to change eg:
> devtools/client/debugger/new/src/utils/breakpoint/astBreakpointLocation.js,
> make does not re-invoke node.py:copy-module.js. We can fix this in your
> node.py by doing something like:
> 
>   subprocess.check_call(cmd)
> +  return set(modules)
> 
> But that will only account for the inputs that we are listing in the
> moz.build file, not any additional files that copy-module.js (or whatever
> the underlying javascript file is) may open.

The only in-tree dependency that copy-module.js uses is devtools/client/debugger/new/build/babel.js.
The rest is only node modules like `fs`, `path`, ...
babel.js is a self contained module without any other deps.

While it makes sense to have `copy-module.js` in `inputs`, I'm not sure about `babel.js`.
Should the node program print on stdout the list of extra deps and have node.py interpret that and return it?
Or is it better to pass babel.js as input and have copy-module.js ignore this argument?

	Alexandre Poirot [:ochameau] Assignee
	Comment 25 • 6 years ago

Here is a new try run with latest patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2a1629676b56f3fa65b0e790ffb26ce399798fd

	Michael Shal [:mshal]
	Comment 26 • 6 years ago

Can you paste your mozconfig? Are your local builds on Linux as well?

	Alexandre Poirot [:ochameau] Assignee
	Comment 27 • 6 years ago

Here is my mozconfig:
# Automatically download and use compiled C++ components:
ac_add_options --enable-artifact-builds

# Write build artifacts to:
mk_add_options MOZ_OBJDIR=./obj-firefox-artifact

mk_add_options AUTOCLOBBER=1

I'm building on Ubuntu 17.10.

	Michael Shal [:mshal]
	Comment 28 • 6 years ago

Thanks for the info - somehow I missed that this was an artifact build. It looks like the problem is not with your moz.build changes, but with the FasterMake backend supporting GENERATED_FILES that have multiple outputs. I filed bug 1473667 and have a patch to fix it. Can you try adding that patch to your stack and let me know if you have any issues with it?

This should reduce the number of node.py invocations significantly, and happens to work around the circular dependency as well. (This is a slightly separate issue, which occurs because 'make ast.js' in the objdir finds the 'ast.js' file in the srcdir on the first run because we have VPATH set. On subsequent runs the message is not present because the file exists in the objdir, so it doesn't get the target mixed up. In bug 1473667, the target is the node.stub file, which doesn't exist in the srcdir, and so we don't hit the match from VPATH, but it doesn't fix the underlying issue).

	Michael Shal [:mshal]
	Comment 29 • 6 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #24)
> (In reply to Michael Shal [:mshal] from comment #23)
> > When I ran this locally, there are ~34 node invocations, and it takes only
> > about 8 seconds to run all of them on my laptop. The incremental build time
> > for building a single directory is under a second. Personally, I think that
> > is really quite reasonable and not worth further optimization. But if you
> > have further patches that bump up the number of invocations significantly,
> > we can look into doing fewer but larger NodeModules() invocations.
> 
> Did you compared without these patches?
> For me, it increased the artifact build time by more than 2x,
> I don't find it reasonable to only build the debugger.
> If other projects start using this, this will explode artifact build time.

As mentioned above, I missed that you were looking at artifact build times. Artifact builds today do not compile anything, they just run python a bunch (~40-50) of times, and copy a bunch of files around. Now that we are actually doing real work to transpile modules during the build, this will necessarily take a hit to the full build time. This is ameliorated somewhat by bug 1473667, but I don't think a direct comparison to the current artifact build is very meaningful since the existing artifact builds don't compile/transpile anything.

I should also mention that the impact on incremental build times from this patch series is negligible, so the time we add to transpile all the javascript is only a factor on the initial build. All subsequent builds should not notice a difference from this (unless of course they are changing javascript files).

In other words, we're going to have to accept some amount of an increase in the initial artifact build times to accommodate running node during the build.

	Michael Shal [:mshal]
	Comment 30 • 6 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #24)
> While it makes sense to have `copy-module.js` in `inputs`, I'm not sure
> about `babel.js`.
> Should the node program print on stdout the list of extra deps and have
> node.py interpret that and return it?

I like this idea, especially if it can be done in a general way so that future javascript programs that we want to invoke via node.py can use the same way to report their dependencies. We could also consider using a dedicated file instead of stdout, like '.deps/node-js.pp', that lists the files read by the javascript file.

> Or is it better to pass babel.js as input and have copy-module.js ignore
> this argument?

While it wouldn't be too hard to do it for this case, I think listing these files manually is likely to be error prone in more complicated scripts.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 31 • 6 years ago

One possible advantage to using a dedicated file:

* it leaves an artifact around that can be used for debugging build system dependency issues

One possible disadvantage to using stdout:

* any node package that doesn't already have an option to this will need to wrap the invocation of that package with something that computes the dependencies.  Given that the dependencies may already be known a priori via some other mechanism, it might be simpler to write a non-wrapping script for that in some cases.  But perhaps this is a premature optimization.

	Alexandre Poirot [:ochameau] Assignee
	Comment 32 • 6 years ago

I rebased my queue against yesterday's tip, but I mush have poorly handled conflicts of Dan's patches as ./mach configure fails:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef6558a998f3119535aa96fe7349004c26720d15&selectedJob=187399589
[task 2018-07-10T15:15:37.427Z] 15:15:37     INFO -  checking for nodejs... not found
[task 2018-07-10T15:15:37.428Z] 15:15:37     INFO -  WARNING: could not find Node.js executable; ensure `node` or `nodejs` is in PATH or set NODEJS in environment to point to an executable
[task 2018-07-10T15:15:37.428Z] 15:15:37     INFO -  WARNING: (This will become a fatal error in the near future.)

Dan, if you have a recent rebase of your patch queue, I'm interested :)
I also included bug 1473667 and locally I can confirm that I get the expected number of nodejs calls!

	Dan Mosedale (:dmosedale, :dmose)
	Comment 33 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bc3be87a1e2722b08dd6a432041828aa24c2c9c has my newly rebased sources; let me know if there are any problems.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 34 • 6 years ago

I've fixed up the rebase such that many (most?) of the try jobs are now passing.  It's likely that python/mozbuild/mozbuild/actions/node.py will need to be edited to replace 

buildconfig.substs['NODE']

with

buildconfig.substs['NODEJS']

to make node.py function correctly.  Please let me know if that's true, and, if so, I'll adjust in the core patches.

	Dan Mosedale (:dmosedale, :dmose)
	Comment 35 • 6 years ago

Oh yeah, and here's the link :-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b973be60c00f33df3b3e4898c4db910dd9c62367

	Alexandre Poirot [:ochameau] Assignee
	Comment 36 • 6 years ago

MozReview-Commit-ID: IGNR0SeufoV

	Jason Laster [:jlast]
	Comment 39 • 6 years ago

here is another try run with dan and alex's node/python work and a smaller debugger migration file. I just tried to migrate the debugger's src dir.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=461ccf47162e78e8d967656a584f77c2c622435f

I apologize for the integration issues. the react build dir, recent protocol, and debugger deprecation issues have made releases a bit manual this week.

	Jason Laster [:jlast]
	Comment 40 • 6 years ago

One observation, we should likely rename NodeModules to PP_DevToolsModules or some other variant

	Jason Laster [:jlast]
	Comment 41 • 6 years ago

looks green sans one py-compat issue

	Dan Mosedale (:dmosedale, :dmose)
	Comment 42 • 6 years ago

The py-compat issue is in a copy of node.py that should no longer be used (i.e. the one in python/mozbuild/mozbuild/action supercedes it), so that file can be removed and the warning will go away.

	Alexandre Poirot [:ochameau] Assignee
	Comment 43 • 6 years ago

Almost green try:
  https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=202892084&revision=db793ede692e72000117a681aa06a023202374b2
(I now removed the node.py file that was causing these warnings, we should be all green on next try!)

	Alexandre Poirot [:ochameau] Assignee
	Comment 44 • 6 years ago

MozReview-Commit-ID: 6jGZ9VKwG2A

	Alexandre Poirot [:ochameau] Assignee
	Comment 45 • 6 years ago

MozReview-Commit-ID: JG9y50g8r4X

	Alexandre Poirot [:ochameau] Assignee
	Comment 46 • 6 years ago

MozReview-Commit-ID: DXYRD1hWarA

	Alexandre Poirot [:ochameau] Assignee
	Comment 47 • 6 years ago

In order to make the landing of this patch easier, debugger sources are going to be landed in a followup bug 1496126.
Here, I'll only land main.js sources to demonstrate a first mandatory usage of node in-tree.

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=911bc5be1900430057c8a369ac61212bb3348549

	Pulsebot
	Comment 48 • 6 years ago

Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4c98ffc7f4f
Introduce DebuggerModules in moz.build to build Debugger.html modules from (jsx) sources r=mshal,jlast
https://hg.mozilla.org/integration/autoland/rev/b37a0f29a956
Land debugger's main module sources r=jlast

	Stefan Hindli [:stefan_hindli]
	Comment 49 • 6 years ago

Backed out 2 changesets (Bug 1461714) for devtools failures in devtools/client/debugger/new/test/mochitest/browser_dbg-asm.js

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&selectedJob=203329191

https://treeherder.mozilla.org/logviewer.html#?job_id=203329191&repo=autoland&lineNumber=4797

	Pulsebot
	Comment 50 • 6 years ago

Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c23c2bd92302
Backed out 2 changesets for devtools failures in  devtools/client/debugger/new/test/mochitest/browser_dbg-asm.js

	Alexandre Poirot [:ochameau] Assignee
	Comment 51 • 6 years ago

It looks like only Linux is working.

On OSX and Windows, JSX/ES-modules are being used in the omni.jar instead of the files generated by node...

	Cristian Brindusan [:cbrindusan]
	Comment 52 • 6 years ago

There are also a few other failures from your push on:
damp - https://treeherder.mozilla.org/logviewer.html#?job_id=203331212&repo=autoland&lineNumber=710
bc - https://treeherder.mozilla.org/logviewer.html#?job_id=203329129&repo=autoland&lineNumber=1203
dt - https://treeherder.mozilla.org/logviewer.html#?job_id=203329128&repo=autoland&lineNumber=967
   - https://treeherder.mozilla.org/logviewer.html#?job_id=203329134&repo=autoland&lineNumber=1046
   - https://treeherder.mozilla.org/logviewer.html#?job_id=203331977&repo=autoland&lineNumber=4796

	Alexandre Poirot [:ochameau] Assignee
	Comment 53 • 6 years ago

It looks like it doesn't fail on artifact builds:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e385f46cbbeb5470e997108c86e23af5e6cfd923

Nor is it about a missing clobber:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=c048288dd3421c58f29df461529fb24326d8091b

So this error is specific to OSX+Windows on non-artifact builds.

	Alexandre Poirot [:ochameau] Assignee
	Comment 54 • 6 years ago

Actually, I've been confused by treeherder results for autoland, it seems to also fail on linux, so this error is more likely to be specific to full build, that, on all platforms.
It failed at least once on this linux pgo job:
  https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b37a0f29a956f16fd757ad0884b9f907f3d15f3c&selectedJob=203335369

To be really sure of that, I just pushed a linux try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=94a14c3f182c274cf0e2a04b516b6b4391d1a52d

I got in touch with Mike and it doesn't reproduce for him.
I'll also give it a try.

	Michael Shal [:mshal]
	Comment 55 • 6 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #54)
> I got in touch with Mike and it doesn't reproduce for him.
> I'll also give it a try.

It looks like it's a parallel build scheduling issue with VPATH. If I build obj-x86_64-pc-linux-gnu/devtools/client/debugger/new/src serially, it installs the main.js built from node.py in the objdir. If I build in parallel, it picks up main.js from the srcdir via VPATH and installs that instead, which is the wrong version. Commenting out VPATH in the generated Makefile in the objdir makes the problem go away.

We must've run into similar issues with VPATH and installed files elsewhere - anyone have some background here?

	Michael Shal [:mshal]
	Comment 56 • 6 years ago

In bug 1461714 we run node.py to generate main.js in the objdir, and use
FINAL_TARGET_FILES to install it into dist. However, when both rules are
run in parallel in the misc tier, the install target may pick up the
main.js file from the srcdir via VPATH, so we end up with the wrong file
in dist. This causes some mochitests to fail with "uncaught exception -
SyntaxError: import declarations may only appear at top level of a
module at
@resource://devtools/client/debugger/new/src/main.js:7:undefined"

The workaround here is to run these node.py invocations (which always
write to node.stub) in the export tier, so that when the install target
in misc is processed, the objdir version of the file is always present
and takes precedence.

A better fix would probably be to remove our reliance on VPATH, and just
pass in the path to files in the srcdir when they are required. That
could potentially be a major overhaul, however.

MozReview-Commit-ID: JZ04C7zJPbX

	Dan Mosedale (:dmosedale, :dmose)
	Comment 57 • 6 years ago

@mshal If I'm understanding correctly, this means that everyone who uses a build file that invokes node.py, at least for the moment, will be required to name the first (stubby) GENERATED_FILE "node.stub"?

	Alexandre Poirot [:ochameau] Assignee
	Comment 58 • 6 years ago

With Mike's patch, try is now green on full builds on all platforms:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae2f1ecae1fd1b64bef47d3b013c331c6a32eb4

Thanks a lot for figuring this out!

	Pulsebot
	Comment 59 • 6 years ago

Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0a184126de4
Introduce DebuggerModules in moz.build to build Debugger.html modules from (jsx) sources r=mshal,jlast
https://hg.mozilla.org/integration/autoland/rev/7d733a368b6d
Land debugger's main module sources r=jlast
https://hg.mozilla.org/integration/autoland/rev/a693ec4fc691
Run node in export; r=ochameau,nalexander

	Michael Shal [:mshal]
	Comment 60 • 6 years ago

(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #57)
> @mshal If I'm understanding correctly, this means that everyone who uses a
> build file that invokes node.py, at least for the moment, will be required
> to name the first (stubby) GENERATED_FILE "node.stub"?

We needed to use the node.stub file anyway because of bug 1471995, so it's likely any other node.py invocations will need to use a stub file for the same reason -- the open file handle passed to the python wrapper isn't easily used by the underlying js that is actually going to create the files.

In this particular case, the VPATH issue happens because the file in the srcdir is the same name as the generated file (eg: main.js), and we use that in a FINAL_TARGET_FILES. So if we use node.py to create a file that's not also in the srcdir, or if it isn't created and then subsequently installed, then we would be ok without a stub file if bug 1471995 were fixed.

	Dorel Luca [:dluca]
	Comment 61 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b0a184126de4
https://hg.mozilla.org/mozilla-central/rev/7d733a368b6d
https://hg.mozilla.org/mozilla-central/rev/a693ec4fc691