Closed Bug 1485081 (node-action) Opened 6 years ago Closed 6 years ago

add moz.build action to generate files with NodeJS

Categories

(Firefox Build System :: Mach Core, enhancement, P1)

Product:
Firefox Build System
Component:
Mach Core
Type:
enhancement

Tracking

(firefox64 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla64
Iteration:
64.3 - Oct 12
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: dmosedale, Assigned: dmosedale, NeedInfo)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 10 obsolete files)

Switch debugger build to use python/ node.py action
1.10 KB, patch
Details | Diff | Splinter Review
Add node.py action for use by moz.build files, v8
8.03 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review 
WIP patch forthcoming
MozReview-Commit-ID: F5iPctcvn9h
MozReview-Commit-ID: DELv9YtoXLW
MozReview-Commit-ID: G0V3zXTZiY2
The activity-stream patch won't be landed as part of this bug; it's here for now to show how to use the code.  It's not yet clear to me whether it's going to make sense to the VCS ignore stuff.
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P1
MozReview-Commit-ID: F5iPctcvn9h
Attachment #9003574 - Flags: feedback?(mshal)
Attachment #9003574 - Flags: feedback?(gps)
Attachment #9002842 - Attachment is obsolete: true
MozReview-Commit-ID: G0V3zXTZiY2
Attachment #9002844 - Attachment is obsolete: true
Attachment #9003575 - Flags: feedback?(mshal)
Attachment #9003575 - Flags: feedback?(gps)
Mike, Greg, these patches are moderately cleaned up.  I still need to write some tests and do some more cleanup, but I wanted to get your thoughts on them before I do that...

Thanks!
MozReview-Commit-ID: F5iPctcvn9h
Attachment #9003574 - Attachment is obsolete: true
Attachment #9003574 - Flags: feedback?(mshal)
Attachment #9003574 - Flags: feedback?(gps)
Alias: node-action
Attachment #9003575 - Flags: feedback?(apoirot)
Comment on attachment 9003663 [details] [diff] [review]
Add node.py action for use by moz.build files, v3

Alex, I've added you for feedback here, because I _think_ this node.py should meet the initial debugger needs.  If you can check to see that it does, that'd be great!
Attachment #9003663 - Flags: feedback?(mshal)
Attachment #9003663 - Flags: feedback?(gps)
Attachment #9003663 - Flags: feedback?(apoirot)
Attachment #9003575 - Flags: feedback?(apoirot)
Comment on attachment 9003575 [details] [diff] [review]
build activity-stream using webpack, v2

>+# This is our dependency list that tells us when to rebuild.  We should be able
>+# to generate the source dependency list (perhaps by just generating it as a
>+# file) and then including it here.
>+inputs = ['webpack.system-addon.config.js',
>+          'content-src/components/Search/Search.jsx',
>+          'content-src/components/Base/Base.jsx']

Is this information known by the script invoked from node.py? You can return a set() from the node.py function that gets invoked from mozbuild, which lists additional inputs to consider dependencies. We could potentially have node scripts return that information to node.py so that it can be included in the .pp file for subsequent builds. Listing this manually should be ok for now, but may result in broken incremental make builds if they aren't specified correctly.

>+# XXX? prepend buildconfig.topsrcdir
>+bundle.flags = ['--', '%s/node_modules/webpack/bin/webpack.js' % TOPSRCDIR, '--config', 'webpack.system-addon.config.js', '--output-path', OBJDIR]

Is something here not working properly? It seems like the XXX comment is already handled here by prepending TOPSRCDIR.

>+# FINAL_TARGET_FILES forces two build invocations when we just want one
>+# file_generate action to produce multiple files.  TODO: Teach FasterMake to
>+# handle tuple outputs better.
>+#
>+# XXX perhaps some of mshal's recent changes have already done the above?

I think this should be fixed by bug 1473667. Can you confirm whether or not you still have issues with FasterMake when that patch is included?
Attachment #9003575 - Flags: feedback?(mshal) → feedback+
Comment on attachment 9003663 [details] [diff] [review]
Add node.py action for use by moz.build files, v3

>+# Invoke yarn.

Is yarn actually used here?

>+WHITELIST_WARNING = '''
>+%s is not
>+in SCRIPT_WHITELIST in python/mozbuild/mozbuild/action/node.py.
>+Using NodeJS from moz.build is currently in beta, and node
>+scripts to be executed need to be added to the whitelist and
>+reviewed by a build peer so that we can get a better sense of
>+how support should evolve.

Cool! Glad to see this here.
Attachment #9003663 - Flags: feedback?(mshal) → feedback+
Comment on attachment 9003663 [details] [diff] [review]
Add node.py action for use by moz.build files, v3

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

::: python/mozbuild/mozbuild/action/node.py
@@ +44,5 @@
> +
> +    def build_node_cmd_list():
> +        # Note the we're ignoring the inputs for now, as our test
> +        # use case has all this stuff specified in webpack.config.
> +        # Subsequent use cases may need better support than this.

I tried to rebase on top of this script, but the lack of inputs passed to node is a deal breaker here.
It is not clear how to accomodate both webpack and debugger usages.

I do need to pass the inputs to the node command, whereas it looks like you don't want to for webpack.

FYI, the current py script I'm using is as simple as that:
```
def generate(output, node_script, *modules):
  node = buildconfig.substs['NODEJS'];
  cmd = [node, node_script]
  cmd.extend(modules)

  subprocess.check_call(cmd)

  return set(modules)
```

@@ +59,5 @@
> +        node_interpreter = buildconfig.substs['NODEJS']
> +
> +        # XXX throw if no script is specified
> +        try:
> +            script_to_run = flagsIter.next()

It may be handy to resolve this path from m-c root to an absolute fs path.

In my current integration, in the mozbuild file I only path absolute path from m-c:
  bundle.inputs = ['/devtools/client/debugger/new/build/copy-module.js']

But with your script, we have to pass the absolute fs path instead and concatenate with TOPSRCDIR.
Note that it would also simplify the definition of `SCRIPT_WHITELIST`.

@@ +61,5 @@
> +        # XXX throw if no script is specified
> +        try:
> +            script_to_run = flagsIter.next()
> +        except StopIteration:
> +            raise ValueError("XXX")

This should be a real error message, I very quickly hit that exception ;)

@@ +96,5 @@
> +        env = dict(os.environ)
> +
> +        subprocess.check_call(node_cmd_list, cwd=cwd, env=env)
> +
> +        return 0

For debugger usecase, we have the return the list of processed modules here, it should be something like `return set(inputs)`.
But as you ignore `inputs` completely, I'm in a dead end starting from build_node_cmd_list.
Attachment #9003663 - Flags: feedback?(apoirot)
I forgot to mention that `script_to_run` is part of flags but it should rather be in the inputs list so that we rebuild everything if it changes.
(In reply to Michael Shal [:mshal] from comment #11)
> Comment on attachment 9003575 [details] [diff] [review]
> build activity-stream using webpack, v2
> 
> >+# This is our dependency list that tells us when to rebuild.  We should be able
> >+# to generate the source dependency list (perhaps by just generating it as a
> >+# file) and then including it here.
> >+inputs = ['webpack.system-addon.config.js',
> >+          'content-src/components/Search/Search.jsx',
> >+          'content-src/components/Base/Base.jsx']
> 
> Is this information known by the script invoked from node.py? You can return
> a set() from the node.py function that gets invoked from mozbuild, which
> lists additional inputs to consider dependencies. We could potentially have
> node scripts return that information to node.py so that it can be included
> in the .pp file for subsequent builds. Listing this manually should be ok
> for now, but may result in broken incremental make builds if they aren't
> specified correctly.

Oooh, this is all very interesting.  In the case of activity-stream right now, it is generated by webpack using globs.  As far as devtools go, I assume they are known there as well.  :ochameau?

You're quite right that the current impl is fragile; for the initial setup, I just wanted to keep things basically working.

So if I'm understanding the above correctly, you're suggesting that a node script could optionally return that informaion to node.py, which would in turn return a set() as well as somehow include it in the .pp files.  How would the .pp file generation happen?  Or is that exactly what returning a set() would trigger?

> >+# XXX? prepend buildconfig.topsrcdir
> >+bundle.flags = ['--', '%s/node_modules/webpack/bin/webpack.js' % TOPSRCDIR, '--config', 'webpack.system-addon.config.js', '--output-path', OBJDIR]
>
> Is something here not working properly? It seems like the XXX comment is
> already handled here by prepending TOPSRCDIR.

This is a leftover comment; I'll remove it.  Sorry for the confusion!
(In reply to Michael Shal [:mshal] from comment #11)
>
> >+# FINAL_TARGET_FILES forces two build invocations when we just want one
> >+# file_generate action to produce multiple files.  TODO: Teach FasterMake to
> >+# handle tuple outputs better.
> >+#
> >+# XXX perhaps some of mshal's recent changes have already done the above?
> 
> I think this should be fixed by bug 1473667. Can you confirm whether or not
> you still have issues with FasterMake when that patch is included?

OK, I'm not sure how to verify that (that's an old comment originally written by Nick :-), but you answered my question.  My assumption is that, given that this was motivated by the debugger build case, it'll be obvious if this is still a problem once we stand up the debugger stuff against this node.py.  So I'll just remove the comment.

Separate but possible related question; do I still need to do have a dummy file (activity_stream_bundle) in the GENERATED_FILES list in order for incremental builds to work?
Flags: needinfo?(mshal)
(In reply to Michael Shal [:mshal] from comment #12)
> Comment on attachment 9003663 [details] [diff] [review]
> Add node.py action for use by moz.build files, v3
> 
> >+# Invoke yarn.
> 
> Is yarn actually used here?

Good catch; no.  This was a leftover comment.
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Comment on attachment 9003663 [details] [diff] [review]
>
> It is not clear how to accomodate both webpack and debugger usages.

Actually, I think this should be straightforward, using the -- syntax that node.py already supports.

> I do need to pass the inputs to the node command, whereas it looks like you
> don't want to for webpack.

We didn't happen to need it for the webpack prototype; passing them through is fine.

> @@ +59,5 @@
> > +        node_interpreter = buildconfig.substs['NODEJS']
> > +
> > +        # XXX throw if no script is specified
> > +        try:
> > +            script_to_run = flagsIter.next()
> 
> It may be handy to resolve this path from m-c root to an absolute fs path.
> 
> In my current integration, in the mozbuild file I only path absolute path
> from m-c:
>   bundle.inputs = ['/devtools/client/debugger/new/build/copy-module.js']
> 
> But with your script, we have to pass the absolute fs path instead and
> concatenate with TOPSRCDIR.
> Note that it would also simplify the definition of `SCRIPT_WHITELIST`.

So if I'm understanding correctly, this input is actually _outside_ of your mozilla-central tree.  Is that right?  And the reason for this is so that the code can be maintained in your github repo?

I don't really have a problem with doing this.  It would push knowledge of TOPSRCDIR out of node.py and into all calling build files.  That might be fine.  :mshal, :gps, do you guys have any opinion here?
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> @@ +61,5 @@
> > +        # XXX throw if no script is specified
> > +        try:
> > +            script_to_run = flagsIter.next()
> > +        except StopIteration:
> > +            raise ValueError("XXX")
> 
> This should be a real error message, I very quickly hit that exception ;)

OK, I've fixed this locally now.

> > +        subprocess.check_call(node_cmd_list, cwd=cwd, env=env)
> > +
> > +        return 0
> 
> For debugger usecase, we have the return the list of processed modules here,
> it should be something like `return set(inputs)`.

So if I'm understanding correctly, you're passing all of the dependencies in from moz.build into node.py, and then you're returning the same set of inputs from this function.  Why?  Is there some change in the list that I'm not understanding?

> But as you ignore `inputs` completely, I'm in a dead end starting from
> build_node_cmd_list.

An easy fix; I'll update the patch shortly.  :-)
(In reply to Dan Mosedale (:dmose) from comment #18) 
> > I do need to pass the inputs to the node command, whereas it looks like you
> > don't want to for webpack.
> 
> We didn't happen to need it for the webpack prototype; passing them through
> is fine.

Wouldn't it break your webpack script?
Because suddently, it will receive the inputs as command line arguments:
  webpack.system-addon.config.js
  content-src/components/Search/Search.jsx
  content-src/components/Base/Base.jsx
(in addition to the one passed via flags:
  config webpack.system-addon.config.js --output-path OBJDIR)

I would expect it confuses your script and this is not clear how to pass/not pass the inputs as arguments from the mozbuild callsite.

> > @@ +59,5 @@
> > > +        node_interpreter = buildconfig.substs['NODEJS']
> > > +
> > > +        # XXX throw if no script is specified
> > > +        try:
> > > +            script_to_run = flagsIter.next()
> > 
> > It may be handy to resolve this path from m-c root to an absolute fs path.
> > 
> > In my current integration, in the mozbuild file I only path absolute path
> > from m-c:
> >   bundle.inputs = ['/devtools/client/debugger/new/build/copy-module.js']
> > 
> > But with your script, we have to pass the absolute fs path instead and
> > concatenate with TOPSRCDIR.
> > Note that it would also simplify the definition of `SCRIPT_WHITELIST`.
> 
> So if I'm understanding correctly, this input is actually _outside_ of your
> mozilla-central tree.  Is that right?  And the reason for this is so that
> the code can be maintained in your github repo?

no. it is in devtools/client/debugger/new.

My suggestion is only to simplify the code here.
Instead of having to do:
  bundle.inputs = ['%s/devtools/client/debugger/new/build/copy-module.js' % TOPSRCDIR]
we could do:
  bundle.inputs = ['/devtools/client/debugger/new/build/copy-module.js']
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> I forgot to mention that `script_to_run` is part of flags but it should
> rather be in the inputs list so that we rebuild everything if it changes.

You're right that currently, in order to get the dependencies right, the moz.build script needs to pass in the script to run twice: both as an input, and as the thing to be invoked after the '--' argument.  This is indeed unnecessarily fragile.  I suppose we could, as you suggest, assert that the last "input" is defined to be the script_to_run.  Another alternative would be to add real argument parsing to node.py, rather than the handrolled stuff its using currently.  Hmm....
(In reply to Dan Mosedale (:dmose) from comment #19)
> > > +        subprocess.check_call(node_cmd_list, cwd=cwd, env=env)
> > > +
> > > +        return 0
> > 
> > For debugger usecase, we have the return the list of processed modules here,
> > it should be something like `return set(inputs)`.
> 
> So if I'm understanding correctly, you're passing all of the dependencies in
> from moz.build into node.py, and then you're returning the same set of
> inputs from this function.  Why?  Is there some change in the list that I'm
> not understanding?

See bug 1461714 comment 23.
(In reply to Dan Mosedale (:dmose) from comment #21)
> (In reply to Alexandre Poirot [:ochameau] from comment #14)
> > I forgot to mention that `script_to_run` is part of flags but it should
> > rather be in the inputs list so that we rebuild everything if it changes.
> 
> You're right that currently, in order to get the dependencies right, the
> moz.build script needs to pass in the script to run twice: both as an input,
> and as the thing to be invoked after the '--' argument.  This is indeed
> unnecessarily fragile.  I suppose we could, as you suggest, assert that the
> last "input" is defined to be the script_to_run.  Another alternative would
> be to add real argument parsing to node.py, rather than the handrolled stuff
> its using currently.  Hmm....

My script was assuming that the first input was the script and the rest was the input files to be processed.
It maps pretty well to the actual command line that is finally invoked:
  $ NODEJS script input-files

I'm starting to wonder if there is really a value in sharing the same py script?
The two usages (babel vs webpack) seems to different... the debugger one is only a 5 line function.
With babel with pass in the list of all files to process and track perfectly the file deps like this,
whereas it is unclear how you are going to track deps with webpack. It may end up being very different and much more complex.
We discussed in bug 1461714 comment 30 about having the nodejs script to output the actual dep list and have it returned by the py function.
(In reply to Dan Mosedale (:dmose) from comment #15)
> (In reply to Michael Shal [:mshal] from comment #11)
> > Is this information known by the script invoked from node.py? You can return
> > a set() from the node.py function that gets invoked from mozbuild, which
> > lists additional inputs to consider dependencies. We could potentially have
> > node scripts return that information to node.py so that it can be included
> > in the .pp file for subsequent builds. Listing this manually should be ok
> > for now, but may result in broken incremental make builds if they aren't
> > specified correctly.
> 
> So if I'm understanding the above correctly, you're suggesting that a node
> script could optionally return that informaion to node.py, which would in
> turn return a set() as well as somehow include it in the .pp files.  How
> would the .pp file generation happen?  Or is that exactly what returning a
> set() would trigger?

Yeah, all node.py needs to do is return a set() of filenames, and file_generate.py takes care of adding them to the appropriate .pp file and the make backend will include them as dependencies. I'm not sure what the best mechanism would be to pass files between the .js script and node.py, but I guess in bug 1461714 I mentioned either using stdout or an intermediate file :)
(In reply to Dan Mosedale (:dmose) from comment #16)
> Separate but possible related question; do I still need to do have a dummy
> file (activity_stream_bundle) in the GENERATED_FILES list in order for
> incremental builds to work?

Unfortunately yes - the first output is treated specially in file_generate.py, since it gets opened automatically as a FileAvoidWrite and passed into the python script (node.py in your case). This is really just an artifact of how the file_generate script evolved over time. We have bug 1471995 on file to fix this, but since the workaround is to just add a stub file and ignore the 'output' argument in node.py, it isn't a high priority at the moment.
Flags: needinfo?(mshal)
Iteration: 63.5 - Sep 3 → 64.2 (Sep 28)
Attachment #9003575 - Flags: feedback?(gps)
Attachment #9003663 - Flags: feedback?(gps)
(In reply to Alexandre Poirot [:ochameau] PTO back on 17th from comment #23)
> I'm starting to wonder if there is really a value in sharing the same py
> script?

I think you're on to something here.  So, the webpack stuff has been a proof-of-concept for us, and it's not yet clear whether that's how we're going to move forward in m-c.  So what I propose to do is that we'll leave much of the code for that out of the final patch, but keep the whitelisting and error-handling code, among other key buts.  We'll make the semantics match (approximately) the semantics of your prototype, modulo suggestions from mshal and other reviewers.

I've pulled your code from the node-debugger bug: the try action mentioned in bug node-debugger Comment 32 <https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef6558a998f3119535aa96fe7349004c26720d15&selectedJob=187399589>.  I can make it build using your node action, but the build explodes and doesn't run.  Is this expected?

This code is, I think, the same as <https://phabricator.services.mozilla.com/D1741>, and specifically <https://phabricator.services.mozilla.com/differential/changeset/?ref=71504&whitespace=ignore-most>.  Is that true?

If so, the discussion that you and mshal have in that code make me think that that's not in a working state and maybe doesn't have the right semantics.  Is that also true?

Once you get that patch using your own node.py into a working state with correct semantics, I have a fairly good idea of how I want to merge the two scripts so that we get something landable in production that has the semantics you need and drops the webpack stuff.

You'll be pleased to know that we're not going to be blocking on --disable-nodejs (see bug 1482433), so as soon as we get this sorted, you'll be able to get start the review/landing process for bug node-debugger.

(Really interested to see if bugzilla is smart enough to hyperlink bug aliases in the text above.  :-)
Flags: needinfo?(apoirot)
(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #26)
> (In reply to Alexandre Poirot [:ochameau] PTO back on 17th from comment #23)
> > I'm starting to wonder if there is really a value in sharing the same py
> > script?
> 
> I think you're on to something here.  So, the webpack stuff has been a
> proof-of-concept for us, and it's not yet clear whether that's how we're
> going to move forward in m-c.  So what I propose to do is that we'll leave
> much of the code for that out of the final patch, but keep the whitelisting
> and error-handling code, among other key buts.  We'll make the semantics
> match (approximately) the semantics of your prototype, modulo suggestions
> from mshal and other reviewers.

SGTM.

> I've pulled your code from the node-debugger bug: the try action mentioned
> in bug node-debugger Comment 32
> <https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ef6558a998f3119535aa96fe7349004c26720d15&selectedJob=1
> 87399589>.  I can make it build using your node action, but the build
> explodes and doesn't run.  Is this expected?

No, you must be based on a different m-c head.

> This code is, I think, the same as
> <https://phabricator.services.mozilla.com/D1741>, and specifically
> <https://phabricator.services.mozilla.com/differential/changeset/
> ?ref=71504&whitespace=ignore-most>.  Is that true?

I would suggest using phabricator patches rather than try runs.
I pushed a rebased version right here with both patches (land debugger sources and tweak moz.builds):
  https://phabricator.services.mozilla.com/D6145

> If so, the discussion that you and mshal have in that code make me think
> that that's not in a working state and maybe doesn't have the right
> semantics.  Is that also true?

I've not worked on the full-featured proposal. i.e. use intermediate deps file or stdout.
Instead, in node.py I used to return the inputs as deps, like this:
  return set(modules)

It covered debugger usecase quite well as it only ignored copy-module.js and babel.js.

But in the latest phabricator patch, I tweaked both py and js to exchance exaustive deps via stdout.
Flags: needinfo?(apoirot)
Attachment #9002843 - Attachment is obsolete: true
Attachment #9003575 - Attachment is obsolete: true
MozReview-Commit-ID: HiBFPn9mMNY
MozReview-Commit-ID: F5iPctcvn9h
Attachment #9003663 - Attachment is obsolete: true
:ochameau - here's a merged pair of patches which seems to build your most recent phabricator patches for the debugger correctly.  They're intended to be applied on top of your patches.  Do they meet your needs?
Flags: needinfo?(poirot.alex)
Comment on attachment 9010119 [details] [diff] [review]
Add node.py action for use by moz.build files, v4

:gps, any comments here?  Assuming you're ok with this setup, and it meets :ochameau's needs, I'll whip up a test or two and put it up for review...
Attachment #9010119 - Flags: feedback?(gps)
Comment on attachment 9010119 [details] [diff] [review]
Add node.py action for use by moz.build files, v4

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

Looks good to me.
I verified, it works great with the debugger!

::: python/mozbuild/mozbuild/action/node.py
@@ +51,5 @@
> +
> +    try:
> +        printable_cmd = ' '.join(pipes.quote(arg) for arg in node_cmd_list)
> +        print('Executing "{}"'.format(printable_cmd), file=sys.stderr)
> +        sys.stderr.flush()

This makes the build quite verbose compared to before this patch.
Do we want to be that verbose?

Also, I was wondering if we could somehow replace these lines:
  0:27.76 node.stub.stub
by something more meaningful, like the directory or files we are actually processing...
Attachment #9010119 - Flags: feedback+
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #32)
> > +    try:
> > +        printable_cmd = ' '.join(pipes.quote(arg) for arg in node_cmd_list)
> > +        print('Executing "{}"'.format(printable_cmd), file=sys.stderr)
> > +        sys.stderr.flush()
> 
> This makes the build quite verbose compared to before this patch.
> Do we want to be that verbose?

I noticed that too.  Although we could simply get rid of that line, I was really wishing things used relative paths more, as that would make the emitted command-lines more skimmable/readable, and then it would be easier to understand/debug how the build was happening.  

From what I can see, those paths are coming from the build-system itself not any of the code in the action nor any of the code in moz.build or the stuff it calls.  I'm not sure whether it's a) reasonable and b) straightforward to make it use relative paths.

:gps/:mshal, any opinions here?

> Also, I was wondering if we could somehow replace these lines:
>   0:27.76 node.stub.stub
> by something more meaningful, like the directory or files we are actually
> processing...

Yeah, that's a good question too.  :mshal?
Flags: needinfo?(mshal)
Flags: needinfo?(gps)
Comment on attachment 9010119 [details] [diff] [review]
Add node.py action for use by moz.build files, v4

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

Overall this looks pretty reasonable.

::: python/mozbuild/mozbuild/action/node.py
@@ +10,5 @@
> +import subprocess
> +import sys
> +
> +
> +SCRIPT_WHITELIST = [

Nit: color-based "*lists" are racially charged. Please use "allowlist" instead.

@@ +53,5 @@
> +        printable_cmd = ' '.join(pipes.quote(arg) for arg in node_cmd_list)
> +        print('Executing "{}"'.format(printable_cmd), file=sys.stderr)
> +        sys.stderr.flush()
> +
> +        # XXX do we actually want to pass through the whole environment?

Yes.

@@ +54,5 @@
> +        print('Executing "{}"'.format(printable_cmd), file=sys.stderr)
> +        sys.stderr.flush()
> +
> +        # XXX do we actually want to pass through the whole environment?
> +        env = dict(os.environ)

And since `env` isn't modified, we don't need the intermediate variable or the argument to `subprocess.check_output()`.

@@ +62,5 @@
> +        # Process the node script output
> +        deps = []
> +        for line in output.splitlines():
> +            if 'dep:' in line:
> +                deps.append(line.replace('dep:', ''))

This feels like specific behavior for `copy-module.js`. I thought we had a way for custom actions to declare an output file containing dependencies. mshal can help you more.

@@ +96,5 @@
> +    """
> +
> +    node_interpreter = buildconfig.substs['NODEJS']
> +
> +    if type(node_script) is not str:

We probably want this to be `node_interpreter = buildconfig.substs.get('NODEJS'); if not node_interpreter:`. (Generally truthiness checking is all that is warranted for most operations in Python.)

@@ +97,5 @@
> +
> +    node_interpreter = buildconfig.substs['NODEJS']
> +
> +    if type(node_script) is not str:
> +        raise ValueError("moz.build file didn't specify a node script")

We need a better error experience here.

Raise an exception will print that traceback to the build system output. That adds no value. We should print a message and exit with non-0 exit code.

And the message that is printed should almost certainly say something about removing --disable-nodejs, since the build system will enforce Node's presence unless that is present. There's an edge case where configure didn't include nodejs.configure (possibly because we're not building Firefox). In that case, we might also want to steer people to file a bug and to include their mozconfig options for debugging.
Attachment #9010119 - Flags: feedback?(gps)
The build system typically resolves things to absolute paths for a hodgepodge of reasons. Assume that won't change.

There /might/ be some environment variables exposing the e.g. directory the build system is processing to invoke this script. But I'm not sure of that. Best way is to dump the environment variables in the build action and see if there's anything useful!

We can easily add environment variables to invoked actions. But we generally keep things simple unless there is a compelling reason to. Most state can be obtained from the buildconfig object, which this patch already uses.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #34)
> Comment on attachment 9010119 [details] [diff] [review]
> 
> Nit: color-based "*lists" are racially charged. Please use "allowlist"
> instead.

Ah, I hadn't thought of that; thanks.  Changed locally.

> > +        # XXX do we actually want to pass through the whole environment?
> > +        env = dict(os.environ)
> 
> And since `env` isn't modified, we don't need the intermediate variable or
> the argument to `subprocess.check_output()`.

Removed locally.

> @@ +62,5 @@
> > +        # Process the node script output
> > +        deps = []
> > +        for line in output.splitlines():
> > +            if 'dep:' in line:
> > +                deps.append(line.replace('dep:', ''))
> 
> This feels like specific behavior for `copy-module.js`. I thought we had a
> way for custom actions to declare an output file containing dependencies.
> mshal can help you more.

Based on mshal's thoughts in Bug 1461714 comment 30, I get the sense that that mechanism hasn't yet been invented, and we need to do so here.  I was assuming that was true, and left Alex's mechanism as-is in the interest of keeping the Node momentum going, as it seemed like a reasonable start that can be refined/changed later if appropriate.  But it wouldn't be particularly hard to change to file.  :mshal, what say you?

> @@ +96,5 @@
> > +    """
> > +
> > +    node_interpreter = buildconfig.substs['NODEJS']
> > +
> > +    if type(node_script) is not str:
> 
> We probably want this to be `node_interpreter =
> buildconfig.substs.get('NODEJS'); if not node_interpreter:`.

Fixed locally.
> @@ +97,5 @@
> > +
> > +    node_interpreter = buildconfig.substs['NODEJS']
> > +
> > +    if type(node_script) is not str:
> > +        raise ValueError("moz.build file didn't specify a node script")

> We need a better error experience here.

> Raise an exception will print that traceback to the build system output. That adds no value. We should print a message and exit with non-0 exit code.

> And the message that is printed should almost certainly say something about removing --disable-nodejs, since the build system will enforce Node's presence unless that is present.

I think that particular message goes with the "NODEJS not set clause" that you mentioned adding, and I've implemented that.

I will also, however, the change "script not found" error to also print a message and exit with a non-zero error code.

> There's an edge case where configure didn't include nodejs.configure (possibly because we're not building Firefox). In that case, we might also want to steer people to file a bug and to include their mozconfig options for debugging.

I'm not sure I understand what you're describing here.  I can see how a project would not include nodejs.configure, but I don't understand then how it would get to the point of being inside the node.py script.  Do you have an example in mind?
Flags: needinfo?(gps)
(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #36)
> (In reply to Gregory Szorc [:gps] from comment #34)
> > @@ +62,5 @@
> > > +        # Process the node script output
> > > +        deps = []
> > > +        for line in output.splitlines():
> > > +            if 'dep:' in line:
> > > +                deps.append(line.replace('dep:', ''))
> > 
> > This feels like specific behavior for `copy-module.js`. I thought we had a
> > way for custom actions to declare an output file containing dependencies.
> > mshal can help you more.
> 
> Based on mshal's thoughts in Bug 1461714 comment 30, I get the sense that
> that mechanism hasn't yet been invented, and we need to do so here.  I was
> assuming that was true, and left Alex's mechanism as-is in the interest of
> keeping the Node momentum going, as it seemed like a reasonable start that
> can be refined/changed later if appropriate.  But it wouldn't be
> particularly hard to change to file.  :mshal, what say you?

The make backend gives file_generate.py a .pp file to use for dependencies, and file_generate.py in turn gets a list of extra dependencies from the invoked action if it returns a set (imported python modules are automatically added as dependencies as well). The part that dmose needs to add here is for a way to have node scripts self-report which files they have accessed so that node.py can return them in the set to file_generate.py and get them included in the deps file. IOW the invocation happens like this:

make -> file_generate.py -> node.py -> foo.js

And then the dependency info goes back up the chain:

foo.js -> [stdout] -> node.py -> [return deps in a set()] -> file_generate.py -> [node.stub.pp]

I don't think we have an existing mechanism separate from this to have the node script write to a .pp file directly. Note that it can't easily use the existing node.stub.pp since file_generate.py needs to use it as well, and both the node and python side of things only have knowledge of a subset of the overall dependencies. I think the stdout communication between node and python should be ok, but if we need to we can change it to an intermediate file.
(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #33)
> (In reply to Alexandre Poirot [:ochameau] from comment #32)
> > Also, I was wondering if we could somehow replace these lines:
> >   0:27.76 node.stub.stub
> > by something more meaningful, like the directory or files we are actually
> > processing...
> 
> Yeah, that's a good question too.  :mshal?

Does this patch seem reasonable?

diff --git a/config/rules.mk b/config/rules.mk
index 5ba50272a4f6c..8daa7858cc4a4 100644
--- a/config/rules.mk
+++ b/config/rules.mk
@@ -31,7 +31,7 @@ ifdef REBUILD_CHECK
 REPORT_BUILD = $(info $(shell $(PYTHON) $(MOZILLA_DIR)/config/rebuild_check.py $@ $^))
 REPORT_BUILD_VERBOSE = $(REPORT_BUILD)
 else
-REPORT_BUILD = $(info $(notdir $@))
+REPORT_BUILD = $(info $(relativesrcdir)/$(notdir $@))
 
 ifdef BUILD_VERBOSE_LOG
 REPORT_BUILD_VERBOSE = $(REPORT_BUILD)

Note that console display of the build output is a somewhat contentious area of the build system, and I wouldn't be surprised if this had some unintended effects. I think this is a reasonable change though since we haven't displayed the Entering/Leaving directory text in a while, so it should be helpful for more than just node.stub lines.

(In case the patch looks confusing because it's putting $(relativesrcdir) in front of a $(notdir) - the $@ here is relative to the directory make is in, and so is usually just stripping the ".deps/" path. It doesn't contain the a path like "devtools/client/debugger/...")
Flags: needinfo?(mshal)
(In reply to Michael Shal [:mshal] from comment #39)
> (In reply to Dan Mosedale (:dmose, :dmosedale) from comment #33)
> > (In reply to Alexandre Poirot [:ochameau] from comment #32)
> > > Also, I was wondering if we could somehow replace these lines:
> > >   0:27.76 node.stub.stub
> > > by something more meaningful, like the directory or files we are actually
> > > processing...
> > 
> > Yeah, that's a good question too.  :mshal?
> 
> Does this patch seem reasonable?
> 

The output from it, at least in this use case, is definitely more readable; thanks!  I don't have a lot of context for evaluating the patch itself, so I'll refrain from comment there.  Would you suggest making this part of this bug, or having it be a spin-off bug?
(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #40)
> The output from it, at least in this use case, is definitely more readable;
> thanks!  I don't have a lot of context for evaluating the patch itself, so
> I'll refrain from comment there.  Would you suggest making this part of this
> bug, or having it be a spin-off bug?

Filed bug 1494038.
See Also: → 1494038
(In reply to Gregory Szorc [:gps] from comment #35)
> The build system typically resolves things to absolute paths for a
> hodgepodge of reasons. Assume that won't change.
> 
> There /might/ be some environment variables exposing the e.g. directory the
> build system is processing to invoke this script. But I'm not sure of that.
> Best way is to dump the environment variables in the build action and see if
> there's anything useful!

The available environment variable that's closest is the output directory deep in the guts of the OBJDIR.  We could derive the srcdir from that, but it seems exceptionally icky.

> We can easily add environment variables to invoked actions. But we generally
> keep things simple unless there is a compelling reason to. Most state can be
> obtained from the buildconfig object, which this patch already uses.

I think what we've got now is good enough, and can file a new bug if we think it should be extended in the future.
MozReview-Commit-ID: F5iPctcvn9h

review: :gps
Attachment #9010119 - Attachment is obsolete: true
Attachment #9012442 - Flags: review?(gps)
Comment on attachment 9012442 [details] [diff] [review]
Add node.py action for use by moz.build files, v5

Alex, since the debugger sources have changed in central could you rebase against this latest patch and (ideally), push to try?  Thanks!
Attachment #9012442 - Flags: feedback?(poirot.alex)
MozReview-Commit-ID: F5iPctcvn9h

feedback: :gps
feedback: :mshal
Attachment #9012442 - Attachment is obsolete: true
Attachment #9012705 - Flags: review?(gps)
Attachment #9012442 - Flags: review?(gps)
Attachment #9012442 - Flags: feedback?(poirot.alex)
If anyone wants to play around with this latest version of the patch, I've got it, together with rebased and reimported debugger sources up at <https://github.com/dmose/gecko/pull/4/commits>.

I'm also running a try build of the whole shebang at:

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

Note, however, that what I'm requesting review on in this bug, and what I want to land here, however, is just the node.py action without all of the debugger stuff, which will land as part of bug 1461714.
Switch node.py to accept both unicode and str literals, so that it can play nice
with both tests and calling code easily.
MozReview-Commit-ID: F5iPctcvn9h

feedback: :gps
feedback: :mshal
Attachment #9012724 - Flags: review?(gps)
Attachment #9012705 - Attachment is obsolete: true
Attachment #9012705 - Flags: review?(gps)
Sorry, I missed your feedback request.
Unfortunately, rebasing against latest github changeset isn't that easy as there is many changes that haven't been upstreamed from m-c to github.

Here is a try build that should hopefully work (there may still be failures related to debugger tests if I've not handled correctly the upstream fixes):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8ca6f46148df33a24d09e72135f94f50055261e

This is based on top of this m-c changeset (I just updated to latest)
  https://hg.mozilla.org/try/rev/dee72baa86979e047fd470effa3ccd553ec27085

Unfortunately, it fails because of:
[task 2018-09-27T21:55:47.017Z] 21:55:47     INFO -  /builds/worker/workspace/build/src/build/moz-automation.mk:89: recipe for target 'automation/package-tests' failed
[task 2018-09-27T21:55:47.017Z] 21:55:47     INFO -  make[1]: *** [automation/package-tests] Error 2
[task 2018-09-27T21:55:47.017Z] 21:55:47     INFO -  client.mk:155: recipe for target 'build' failed

Is that something you fixed in the latest update?
I hacked around a few changes related to a missing context.js file after doing the rebasing.

I'm seeing that in my try builds from comment 48 on both Mac and Linux (but not windows).  It's not obvious to me that that's related to our changes at all.  The error message is awfully vague...
So this error actually comes from "make package-tests", which calls "make install-test-files", and then errors out like this:

/Users/dmose/r/git-cin-mc/obj-art-opt/_virtualenvs/init/bin/python -m mozbuild.action.process_install_manifest --track install__test_files.track _tests _build_manifests/install/_test_files
Traceback (most recent call last):
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/runpy.py", line 72, in _run_code
    exec code in run_globals
  File "/Users/dmose/r/git-cin-mc/python/mozbuild/mozbuild/action/process_install_manifest.py", line 113, in <module>
    main(sys.argv[1:])
  File "/Users/dmose/r/git-cin-mc/python/mozbuild/mozbuild/action/process_install_manifest.py", line 100, in main
    defines=args.defines)
  File "/Users/dmose/r/git-cin-mc/python/mozbuild/mozbuild/action/process_install_manifest.py", line 69, in process_manifest
    remove_empty_directories=remove_empty_directories)
  File "/Users/dmose/r/git-cin-mc/python/mozbuild/mozpack/copier.py", line 430, in copy
    copy_results.append((destfile, f.copy(destfile, skip_if_older)))
  File "/Users/dmose/r/git-cin-mc/python/mozbuild/mozpack/files.py", line 340, in copy
    raise ErrorMessage('Symlink target path does not exist: %s' % self.path)
mozpack.errors.ErrorMessage: Symlink target path does not exist: /Users/dmose/r/git-cin-mc/devtools/client/debugger/new/test/mochitest/examples/react/build/main.js
make: *** [install-test-files] Error 1

I can reproduce this locally.
(The make commands above need to be executed inside the objdir)
Seems like the error message makes it pretty clear that it's somehow related to the interplay between what's on github and what's in mozilla-central, particularly around devtools/client/debugger/new/test/mochitest/examples/react/build/main.js.
Him, that's an interesting find. We should already have that file around. It also does not need to be built. Infact nothing in the mochitest dir needs to be built
I've removed a couple of files from devtools/client/debugger/new/test/mochitest/browser.ini, and this makes the package-tests step work correctly on my local tree:

diff --git a/devtools/client/debugger/new/test/mochitest/browser.ini b/devtools/client/debugger/new/test/mochitest/browser.ini
index 8f9d11bda4637..8d6a94dfe56bc 100644
--- a/devtools/client/debugger/new/test/mochitest/browser.ini
+++ b/devtools/client/debugger/new/test/mochitest/browser.ini
@@ -581,8 +581,6 @@ support-files =
   examples/sourcemaps-reload/doc-sourcemaps-reload.html
   examples/sourcemaps-reload/doc-sourcemaps-reload2.html
   examples/sourcemaps-reload/doc-sourcemaps-reload3.html
-  examples/react/build/main.js
-  examples/react/build/main.js.map
   examples/doc-react.html
   examples/wasm-sourcemaps/fib.c
   examples/wasm-sourcemaps/fib.wasm
(In reply to Jason Laster [:jlast] from comment #54)
> Him, that's an interesting find. We should already have that file around. It
> also does not need to be built. Infact nothing in the mochitest dir needs to
> be built

Could it be that the error message is trying to say that the problem is actually with the destination directory where the symlink to the source file is supposed to be placed?  I.e. perhaps that that dir doesn't actually exist?
Oh, Thanks Dan for looking into this in detail.

Here is another try build with fixes against these test files:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e93c3efa282d24a761b5a348fa55553498db7c3

It comes from bug 1493151 which added these two files, but they aren't available on github:
  https://github.com/devtools-html/debugger.html/tree/master/src/test/mochitest/examples/react
As I completely override the src folder with the one from github, these two files disappeared. So I restored them manually in the latest patch queue. It is just yet another issue in m-c versus github synchronization. (I've not seen any script, nor any note in debugger.html mentioning any manual step to synchronize these specific test files when releasing an update from github to m-c?)
Yep, this is a mistake on our end. We need to build them and check them into tree in github.
Looking at alex's try run, I believe node-action is good to land. The try issues are a debugger integration issue, which can be fixed when we land node-debugger (https://bugzilla.mozilla.org/show_bug.cgi?id=1461714).
Iteration: 64.2 (Sep 28) → 64.3 (Oct 12)
Comment on attachment 9012724 [details] [diff] [review]
Add node.py action for use by moz.build files, v7

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

This seems reasonable to me as a first implementation!

::: python/mozbuild/mozbuild/action/node.py
@@ +99,5 @@
> +            have --disable-nodejs in it.  If it does, try removing that line
> +            and building again.""", file=sys.stderr)
> +        sys.exit(1)
> +
> +    if type(node_script) is not unicode and type(node_script) is not str:

if not isinstance(node_script, (str, unicode)):
Attachment #9012724 - Flags: review?(gps) → review+
MozReview-Commit-ID: F5iPctcvn9h
Attachment #9012724 - Attachment is obsolete: true
Attachment #9013434 - Flags: review+
Pushed by dmosedale@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6394f3198d88
Add node.py action for use by moz.build files, r=gps
https://hg.mozilla.org/mozilla-central/rev/6394f3198d88
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: