Migrate `make.js` to `gulpfile.js` #8349

timvandermeij · 2017-04-27T21:28:13Z

It is easier to review each patch on its own since the overall diff is slightly hard to read because of the last commit.

Testing the mozcentralbaseline/mozcentraldiff conversion is done by running BASELINE=17d135f gulp mozcentralbaseline mozcentraldiff without and with these patches applied and checking that the resulting diffs are equal in size.

Note that the tasks properly depend on each other now, so running BASELINE=17d135f gulp mozcentraldiff gives the same result as the command above since mozcentraldiff depends on mozcentralbaseline to run.

Snuffleupagus · 2017-04-28T08:41:26Z

Unfortunately this fails on Windows, during mozcentralpre with "ReferenceError: ls is not defined" in

pdf.js/external/builder/builder.js

Lines 285 to 298 in 71bbcfa

    
           setup.preprocess.forEach(function(option) { 
        
             var sources = option[0]; 
        
             var destination = option[1]; 
        
             sources = ls('-R', sources); 
        
             sources.forEach(function(source) { 
        
               // ??? Warn if the source is wildcard and dest is file? 
        
               var destWithFolder = destination; 
        
               if (test('-d', destination)) { 
        
                 destWithFolder += '/' + path.basename(source); 
        
               } 
        
               preprocess(source, destWithFolder, defines); 
        
             }); 
        
           });

.
As far as I can ascertain, builder.js implicitly depend on some methods (e.g. ls) from ShellJS, and with make.js now being removed those are no longer available. Hence I think that you'll you probably need to rewrite that part of builder.js too as part of these patches.

yurydelendik · 2017-04-28T13:22:14Z

The code in the builder.js above just expanding wildcards. I think we can remove that since preprocessor is/must be working with particular/known files.

yurydelendik · 2017-04-28T13:26:22Z

gulpfile.js

+        spawnSync('git', ['add', '.'], {cwd: MOZCENTRAL_BASELINE_DIR});
+        spawnSync('git', ['commit', '-m', 'mozcentral baseline'],
+                  {cwd: MOZCENTRAL_BASELINE_DIR});
+      });


We need to use done() function here (and below) since this task is async.

yurydelendik · 2017-04-28T19:21:44Z

We also have .to() usage at "minified-post", can you convert it to fs.writeFileSync() ?

timvandermeij · 2017-04-28T23:32:27Z

There is one Windows issue to resolve with @Snuffleupagus before we can land this.

Snuffleupagus · 2017-04-29T16:14:46Z

I've been attempting to debug this further today, but unfortunately I've been unable to make any progress :-( The main problem is that I know next to nothing (or at least very little) about Node.js, so I'm not sure what steps to take.

In case it's useful: I can successfully run BASELINE=<commit hash> gulp baseline manually (with these patches applied), but as soon as I attempt to run e.g. BASELINE=<commit hash> gulp mozcentralbaseline it fails (as outlined on IRC yesterday).

It would probably be good if someone else could test this on Windows, just to check that it's indeed a platform specific issue, and not my configuration that is somehow a special snowflake.

timvandermeij · 2017-04-29T17:19:24Z

I have a feeling it's related to Windows treating quotes in commands differently than Linux does. Related sites:

child_process.spawn fails on Windows given a space in both the command and an argument nodejs/node#7367
child_process.spawn fails on Windows given a space in both the command and an argument nodejs/node-v0.x-archive#25895
http://stackoverflow.com/a/12313826

Snuffleupagus · 2017-04-29T18:18:48Z

I have a feeling it's related to Windows treating quotes in commands differently than Linux does.

While this is probably true, I can't help thinking that part of the issue may occur even before that.
If I run only gulp baseline on its own (with these patches applied), everything works as it should, and I get this result:

$ BASELINE=f91d01ca gulp baseline
[20:02:26] Using gulpfile ~\Git\pdfjs\gulpfile.js
[20:02:26] Starting 'baseline'...

### Creating baseline environment
Baseline commit "f91d01ca" checked out.
[20:02:37] Finished 'baseline' after 11 s

If I instead run gulp mozcentralbaseline, I get this result:

$ BASELINE=f91d01ca gulp mozcentralbaseline
[20:04:49] Using gulpfile ~\Git\pdfjs\gulpfile.js
[20:04:49] Starting 'mozcentralbaseline'...

### Creating mozcentral baseline environment
Error: command "git" with parameters "commit,-m,mozcentral baseline" exited with
 code 1
Output:

In this case, since gulp mozcentralbaseline should trigger gulp baseline (called from this line), I'd expect to also see the output from that build target (as in the first console excerpt above) and the output from gulp mozcentral as well (called from this line).
The fact that no console output is present, nor any baseline/mozcentral folders created in the /build directory, makes me suspect that neither of those two commands are actually being run!

Could the main issue here be that the spawnSync commands added in the patches isn't Windows compatible!?

Snuffleupagus · 2017-04-29T19:37:19Z

It seems that if I make a couple of small adjustments, based on skimming through the links in #8349 (comment), the follow diff actually seem to work!

diff --git a/gulpfile.js b/gulpfile.js
index 0058fb94..c6ed3e6e 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -1361,7 +1361,8 @@ gulp.task('mozcentralbaseline', function (done) {
   console.log();
   console.log('### Creating mozcentral baseline environment');
 
-  safeSpawnSync('gulp', ['baseline'], {env: process.env});
+  safeSpawnSync('gulp', ['baseline'],
+                {env: process.env, shell: true, stdio: 'inherit'});
 
   // Create a mozcentral build. We have to pass the working directory to the
   // Gulp process using the `--cwd` option to override the behavior of Gulp
@@ -1370,7 +1371,7 @@ gulp.task('mozcentralbaseline', function (done) {
     rimraf.sync(BASELINE_DIR + BUILD_DIR);
   }
   safeSpawnSync('gulp', ['mozcentral', '--cwd', BASELINE_DIR],
-                {env: process.env});
+                {env: process.env, shell: true, stdio: 'inherit'});
 
   // Copy the mozcentral build to the mozcentral baseline directory.
   if (checkDir(MOZCENTRAL_BASELINE_DIR)) {
@@ -1394,7 +1395,8 @@ gulp.task('mozcentraldiff', ['mozcentral'], function (done) {
   console.log();
   console.log('### Creating mozcentral diff');
 
-  safeSpawnSync('gulp', ['mozcentralbaseline'], {env: process.env});
+  safeSpawnSync('gulp', ['mozcentralbaseline'],
+                {env: process.env, shell: true, stdio: 'inherit'});
 
   // Create the diff between the current mozcentral build and the
   // baseline mozcentral build, which both exist at this point.

timvandermeij · 2017-04-29T20:48:08Z

The shell: true option passing is also something I read there, but I couldn't test it, so thank you for testing that!

We should probably pass this to every command, so I'll try to solve it in the safeSpawnSync function.

timvandermeij · 2017-04-29T22:27:41Z

I have updated the patch. The safeSpawnSync function now takes care of adding the shell option if required, which I explained in the comment. It's somewhat unfortunate that platform detection is necessary here, but at least it's located in one single spot and the choice is explained, so we should be good.

@Snuffleupagus Could you test this again on Windows to make sure all is well now?

Snuffleupagus · 2017-04-29T22:56:47Z

@Snuffleupagus Could you test this again on Windows to make sure all is well now?

Unfortunately, that doesn't work :-(
For one thing, you need to set the stdio: 'inherit' option in order get full console output from the spawned tasks on Windows. Since that option is already being using in various places in gulpfile.js, I'd suggest simply appending that regardless of platform.

However, even with that adjusted, I'm now getting the same Error: command "git" with parameters "commit,-m,mozcentral baseline" exited with code 1 message again.

@timvandermeij Since I could swear that a previous version worked, once I applied the changes described in #8349 (comment), do you have an interdiff for the last round of changes?

Edit: If I didn't know better, I'd say that you probably need to pass in stdio: inherit at every place that you invoke spawn/spawnSync in order for things to work correctly.

timvandermeij · 2017-04-29T23:16:04Z

The stdio: inherit option breaks things for me on Linux at least. For example, https://github.com/mozilla/pdf.js/pull/8349/files#diff-b9e12334e9eafd8341a6107dd98510c9R1420 breaks because the stdout is no longer captured there, but sent back to the main Node.js process running the Gulp task. As a result, the stdout is then empty there, which causes the task to stop with an error. Hence I don't think we can/should use stdio: inherit by default (even though in this particular case we can probably work around it).

I only added the code from https://github.com/mozilla/pdf.js/pull/8349/files#diff-b9e12334e9eafd8341a6107dd98510c9R87 to line 94. No other changes were made. Maybe the platform test gives a different result for you, and the shell: true option is not added?

Note that if this doesn't work out, I have an alternative approach to all of this. I'm just hoping to avoid it if we can make this work with minimal changes.

yurydelendik · 2017-04-29T23:49:19Z

I'm thinking gulp does not allow run gulp. Can we replace spawnSync gulp with calling tasks? I'm thinking we don't have to have a special version of spawnSync for Windows.

Also rimraf's don't need checkDir's

Snuffleupagus · 2017-04-29T23:49:41Z

To get everything to work, and to also get meaningful console output, the following diff works for me (note that it doesn't touch the part that you out pointed out as problematic w.r.t. stdio: 'inherit'):

diff --git a/gulpfile.js b/gulpfile.js
index 16447700..8e79d372 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -1369,7 +1369,7 @@ gulp.task('mozcentralbaseline', function (done) {
   console.log();
   console.log('### Creating mozcentral baseline environment');
 
-  safeSpawnSync('gulp', ['baseline'], {env: process.env});
+  safeSpawnSync('gulp', ['baseline'], {env: process.env, stdio: 'inherit'});
 
   // Create a mozcentral build. We have to pass the working directory to the
   // Gulp process using the `--cwd` option to override the behavior of Gulp
@@ -1377,8 +1377,9 @@ gulp.task('mozcentralbaseline', function (done) {
   if (checkDir(BASELINE_DIR + BUILD_DIR)) {
     rimraf.sync(BASELINE_DIR + BUILD_DIR);
   }
-  safeSpawnSync('gulp', ['mozcentral', '--cwd', BASELINE_DIR],
-                {env: process.env});
+  var workingDirectory = path.resolve(process.cwd(), BASELINE_DIR);
+  safeSpawnSync('gulp', ['mozcentral'],
+                {env: process.env, cwd: workingDirectory, stdio: 'inherit'});
 
   // Copy the mozcentral build to the mozcentral baseline directory.
   if (checkDir(MOZCENTRAL_BASELINE_DIR)) {
@@ -1392,7 +1393,7 @@ gulp.task('mozcentralbaseline', function (done) {
         // Commit the mozcentral baseline.
         safeSpawnSync('git', ['init'], {cwd: MOZCENTRAL_BASELINE_DIR});
         safeSpawnSync('git', ['add', '.'], {cwd: MOZCENTRAL_BASELINE_DIR});
-        safeSpawnSync('git', ['commit', '-m', 'mozcentral baseline'],
+        safeSpawnSync('git', ['commit', '-m', '"mozcentral baseline"'],
                       {cwd: MOZCENTRAL_BASELINE_DIR});
         done();
       });
@@ -1402,7 +1403,8 @@ gulp.task('mozcentraldiff', ['mozcentral'], function (done) {
   console.log();
   console.log('### Creating mozcentral diff');
 
-  safeSpawnSync('gulp', ['mozcentralbaseline'], {env: process.env});
+  safeSpawnSync('gulp', ['mozcentralbaseline'],
+                {env: process.env, stdio: 'inherit'});
 
   // Create the diff between the current mozcentral build and the
   // baseline mozcentral build, which both exist at this point.

timvandermeij · 2017-05-01T21:57:32Z

I reverted the changes in safeSpawnSync and used the solution provided by @Snuffleupagus instead since that also works for me and looks like the best solution to get this working (and also the least amount of code). Please test this again and so we can hopefully resolve this.

Snuffleupagus · 2017-05-01T22:09:29Z

Please test this again and so we can hopefully resolve this.

Almost, but on Windows shell: true is still needed. Really sorry if I wasn't entirely clear, but the diff in #8349 (comment) was based the old commit that included this!

However, could the questions/suggestions in #8349 (comment) help remove the need for calling gulp in this way completely?

timvandermeij · 2017-05-01T22:33:44Z

The fix for Windows is back in place. Fortunately it's only a few lines, so I'm not too worried about it. I have also removed the unnecessary checks around rimraf. I tried calling Gulp tasks instead, but I couldn't find out how to do that properly in a way to also pass e.g, the current working directory to the task (which is easy in the context of spawning a process). Methods like calling gulp.run or gulp.task all appear to be deprecated, and calling tasks is usually done by running them before or after the task, which is not what we want here as we want to call it from inside the task.

yurydelendik · 2017-05-01T22:35:02Z

gulpfile.js

-  if (taskName in gulp.tasks) {
-    continue;
-  }
+  safeSpawnSync('gulp', ['baseline'], {env: process.env, stdio: 'inherit'});


Make this as a dependency for mozcentralbaseline task instead.

Done in the new commit.

yurydelendik · 2017-05-01T22:35:47Z

gulpfile.js

+  }
+
+  var result = spawnSync(command, parameters, options);
+  if (result.status) {


change it to result.status !== 0 ?

spawnSync may exit with result status null, which is a valid value according to https://github.com/nodejs/node/pull/11288/files (see the IRC discussion at http://logs.glob.uno/?c=mozilla%23pdfjs&s=28+Apr+2017&e=28+Apr+2017#c60549). Such a check would disallow null, so we chose for this. Note that we already have this at the moment in https://github.com/mozilla/pdf.js/blob/master/make.js#L31.

I think null is code of error/termination, e.g. ctrl+c.

You're absolutely right, now that I look at the code again! Done in the new commit.

yurydelendik · 2017-05-01T22:37:51Z

gulpfile.js

@@ -81,6 +83,22 @@ var DEFINES = {
  PDFJS_NEXT: false,
 };

+function safeSpawnSync(command, parameters, options) {
+  // Windows has problems executing commands outside a shell.
+  if (process.platform === 'win32') {


do we need it for all commands or for some?
can we add shell option for certain command on all platforms?

It seems to me that it's mainly an issue when spawning a gulp task, but otherwise I think it's fine without it.

I chose to solve it in safeSpawnSync to prevent issues for other (or future) commands. It's not entirely clear to me if Windows requires it all the time or just for some commands, so in this way we do not have to duplicate it and prevent any issues.

The shell option does work on Linux as well after adjusting the stdio behavior, so we can indeed enable it on all platforms to not make it Windows-specific anymore. Done in the new commit.

The baseline fix is dead code since three years, so we can safely remove it.

To run the regression tests, developers use `gulp browsertest` and the bot uses `gulp bottest`. We're not passing the `noreftest` option anywhere in the code (probably because the `bottest` command takes care of this already), so we should remove this.

Note that we have to use `fs.writeFileSync` since `.to()` is not available anymore. Moreover, introduce `safeSpawnSync` to make sure that we check the return codes of the spawned processes properly.

Snuffleupagus

This works just fine on Windows now; thank you for seeing this through to the end, and sorry about all the back-and-fourth here!

timvandermeij · 2017-05-01T23:13:07Z

Thank you both for reviewing this!

Migrate `make.js` to `gulpfile.js`

timvandermeij added the other label Apr 27, 2017

yurydelendik reviewed Apr 28, 2017

View reviewed changes

timvandermeij force-pushed the remove-make branch 6 times, most recently from d69ffd6 to 9a54834 Compare April 28, 2017 22:37

timvandermeij force-pushed the remove-make branch from 9a54834 to 1c75ce5 Compare April 29, 2017 22:25

timvandermeij force-pushed the remove-make branch from 1c75ce5 to f5611d3 Compare May 1, 2017 21:54

timvandermeij force-pushed the remove-make branch from f5611d3 to 71ba8fa Compare May 1, 2017 22:27

yurydelendik approved these changes May 1, 2017

View reviewed changes

yurydelendik reviewed May 1, 2017

View reviewed changes

timvandermeij added 3 commits May 2, 2017 00:45

Port the mozcentralbaseline target to Gulp

74854fb

The baseline fix is dead code since three years, so we can safely remove it.

Port the mozcentraldiff target to Gulp

19cc9bc

timvandermeij force-pushed the remove-make branch from 71ba8fa to f73b6e5 Compare May 1, 2017 22:51

Remove make.js and the target fetching in gulpfile.js

f748407

Note that we have to use `fs.writeFileSync` since `.to()` is not available anymore. Moreover, introduce `safeSpawnSync` to make sure that we check the return codes of the spawned processes properly.

timvandermeij force-pushed the remove-make branch from f73b6e5 to f748407 Compare May 1, 2017 23:01

Snuffleupagus approved these changes May 1, 2017

View reviewed changes

timvandermeij merged commit 0c99429 into mozilla:master May 1, 2017

timvandermeij deleted the remove-make branch May 1, 2017 23:12

yurydelendik added a commit that referenced this pull request May 3, 2017

Fixes shell parameters quoting after #8349

2ac4106

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8349 from timvandermeij/remove-make

b159531

Migrate `make.js` to `gulpfile.js`

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Fixes shell parameters quoting after mozilla#8349

b898cfb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `make.js` to `gulpfile.js` #8349

Migrate `make.js` to `gulpfile.js` #8349

timvandermeij commented Apr 27, 2017 •

edited

Loading

Snuffleupagus commented Apr 28, 2017 •

edited

Loading

yurydelendik commented Apr 28, 2017 •

edited

Loading

yurydelendik Apr 28, 2017

yurydelendik commented Apr 28, 2017

timvandermeij commented Apr 28, 2017 •

edited

Loading

Snuffleupagus commented Apr 29, 2017

timvandermeij commented Apr 29, 2017 •

edited

Loading

Snuffleupagus commented Apr 29, 2017 •

edited

Loading

Snuffleupagus commented Apr 29, 2017

timvandermeij commented Apr 29, 2017 •

edited

Loading

timvandermeij commented Apr 29, 2017

Snuffleupagus commented Apr 29, 2017 •

edited

Loading

timvandermeij commented Apr 29, 2017 •

edited

Loading

yurydelendik commented Apr 29, 2017

Snuffleupagus commented Apr 29, 2017

timvandermeij commented May 1, 2017

Snuffleupagus commented May 1, 2017

timvandermeij commented May 1, 2017 •

edited

Loading

yurydelendik May 1, 2017

timvandermeij May 1, 2017 •

edited

Loading

yurydelendik May 1, 2017

timvandermeij May 1, 2017

yurydelendik May 1, 2017

timvandermeij May 1, 2017 •

edited

Loading

yurydelendik May 1, 2017

Snuffleupagus May 1, 2017

timvandermeij May 1, 2017

timvandermeij May 1, 2017

Snuffleupagus left a comment •

edited

Loading

timvandermeij commented May 1, 2017

Migrate make.js to gulpfile.js #8349

Migrate make.js to gulpfile.js #8349

Conversation

timvandermeij commented Apr 27, 2017 • edited Loading

Snuffleupagus commented Apr 28, 2017 • edited Loading

yurydelendik commented Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

yurydelendik commented Apr 28, 2017

timvandermeij commented Apr 28, 2017 • edited Loading

Snuffleupagus commented Apr 29, 2017

timvandermeij commented Apr 29, 2017 • edited Loading

Snuffleupagus commented Apr 29, 2017 • edited Loading

Snuffleupagus commented Apr 29, 2017

timvandermeij commented Apr 29, 2017 • edited Loading

timvandermeij commented Apr 29, 2017

Snuffleupagus commented Apr 29, 2017 • edited Loading

timvandermeij commented Apr 29, 2017 • edited Loading

yurydelendik commented Apr 29, 2017

Snuffleupagus commented Apr 29, 2017

timvandermeij commented May 1, 2017

Snuffleupagus commented May 1, 2017

timvandermeij commented May 1, 2017 • edited Loading

Choose a reason for hiding this comment

timvandermeij May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

timvandermeij commented May 1, 2017

Migrate `make.js` to `gulpfile.js` #8349

Migrate `make.js` to `gulpfile.js` #8349

timvandermeij commented Apr 27, 2017 •

edited

Loading

Snuffleupagus commented Apr 28, 2017 •

edited

Loading

yurydelendik commented Apr 28, 2017 •

edited

Loading

timvandermeij commented Apr 28, 2017 •

edited

Loading

timvandermeij commented Apr 29, 2017 •

edited

Loading

Snuffleupagus commented Apr 29, 2017 •

edited

Loading

timvandermeij commented Apr 29, 2017 •

edited

Loading

Snuffleupagus commented Apr 29, 2017 •

edited

Loading

timvandermeij commented Apr 29, 2017 •

edited

Loading

timvandermeij commented May 1, 2017 •

edited

Loading

timvandermeij May 1, 2017 •

edited

Loading

timvandermeij May 1, 2017 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading