Add tests for polymer-cli binary #48

Zetten · 2017-11-01T11:37:23Z

Attempting to build genrules based on a node_binary for polymer-cli results in errors at several points - initially related to @-scoped module names, but also the TARGET_PATH determination for the binary itself (exposed when a WORKSPACE name is set, and the magic __main__ path is no longer present).

This branch introduces a (failing) test using polymer-cli, similar to the webpack test, and then attempts to resolve the issues.

Zetten · 2017-11-01T11:58:41Z

The polymer init test is apparently not really appropriate for Travis - it was a gamble anyway as I have local issues due to an http_proxy.

I'll try to figure out a better test.

pcj · 2017-11-01T13:20:58Z

Thanks for this @Zetten, polymer-cli will be a good test / complex test case.

Zetten · 2017-11-06T13:25:25Z

The upcoming bazel 0.8.0 release includes the following change:

runfiles, sh: Shell scripts may now depend on
//src/tools/runfiles:runfiles_sh_lib and source runfiles.sh. The
script defines the rlocation function which returns runfile
paths on every platform.

In addition to $(location), Bazel now also supports $(rootpath)
to obtain
the root-relative path (i.e., for runfiles locations), and
$(execpath) to
obtain the exec path (i.e., for build-time locations)

I think this might make for a simpler node_binary wrapper script. I'll disable my failing test to get this PR moving, and see whether it can be improved further once 0.8.0 lands.

pcj · 2017-11-06T15:06:47Z

Thanks Peter. I'm traveling until Wednesday and will do a more thorough review then.

…

On Mon, Nov 6, 2017 at 6:25 AM, Peter van Zetten ***@***.***> wrote: The upcoming bazel 0.8.0 release includes the following change: - runfiles, sh: Shell scripts may now depend on //src/tools/runfiles:runfiles_sh_lib and source runfiles.sh. The script defines the rlocation function which returns runfile paths on every platform. - In addition to $(location), Bazel now also supports $(rootpath) to obtain the root-relative path (i.e., for runfiles locations), and $(execpath) to obtain the exec path (i.e., for build-time locations) I think this might make for a simpler node_binary wrapper script. I'll disable my failing test to get this PR moving, and see whether it can be improved further once 0.8.0 lands. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#48 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADFlGMjv9xP5E8vd3jHr8nJbjlDauSiks5szwjFgaJpZM4QOFT7> .

Zetten · 2017-11-08T11:00:24Z

Only one of the OS X Travis builds failed, due to hitting the time limit.

I'm not able to retrigger, but I suspect it's unrelated to the change itself.

pcj

Overall I think this looks good, and it's awesome to see it working. I'm ready to merge this, but I'm hoping you can add one last commit to provide a little more explanation for future people to understand how it works. See my specific comments.

pcj · 2017-11-08T15:57:38Z

node/internal/parse_yarn_lock.js

@@ -26,16 +26,17 @@ function main() {
  //
  const entries = Object.keys(yarn.object).map(key => makeYarnEntry(key, yarn.object[key]));

-  // For all top-level folders in the node_modules directory that
+  // For all top-level or @-scoped folders in the node_modules directory that
  // contain a package.json file...


Can you provide a bit more detail here about the change for future people looking at the code? What was it about the deps or structure of the polymer-cli that made this change necessary?

I think the behaviour is actually what was originally intended and documented. That is, it's not specific to polymer-cli, it was simply (I think) an oversight in the handling of @-scoped dependencies which meant they were omitted from the generated BUILD.bazel.

I'm happy to add more comments if needed though.

pcj · 2017-11-08T15:58:51Z

node/internal/parse_yarn_lock.js

    print(`    version = "${module.version}",`);
    print(`    package_json = "node_modules/${module.name}/package.json",`);
-    print(`    srcs = glob(["node_modules/${module.name}/**/*"], exclude = ["node_modules/${module.name}/package.json"]),`);
+    print(`    srcs = glob(["node_modules/${module.name}/**/*"], exclude = ["node_modules/${module.name}/package.json", "**/* *"]),`);


Can you comment a bit more on the intent here (what files do we want?) I think it will be confusing to future people looking at it.

This one I'm not sure about.

This was actually a crude workaround to deal with the fact that bazel doesn't like filenames with spaces, and some-dep-I-can't-remember in the polymer-cli tree has such a file.

Ironically it looked like it was a module's test data for filenames with spaces, so my initial instinct was to exclude the test/** tree from the imported modules.

But then I realised anything dealing with that should probably be more firmly dealt with (and documented), e.g. the Gradle client-dependencies-plugin which includes/excludes specific stuff from the imported modules.

clientDependencies { fileExtensions = ['css', 'js', 'eot', 'svg', 'ttf', 'woff', 'woff2', 'ts', 'jpg', 'jpeg', 'png', 'gif'] // <1> releaseFolders = ['dist', 'release'] // <2> copyIncludes = [] // <3> copyExcludes = ['**/*.min.js', '**/*.min.css', '**/*.map', '**/Gruntfile.js', 'gulpfile.js', 'source/**'] // <4> }

A solution for this in rules_node would have to allow the user to override this behaviour (both generally and for specific modules), so it gets a bit complicated and probably deserves its own PR.

So yeah, the end result was "just exclude files which have a space in the name" - which gets us past the blocker for polymer-cli, with the caveat that we hope none of our dependencies have such files in their real sources (which would otherwise if included cause bazel to choke anyway).

If you're happy with this approach I'll add another comment to the code.

Zetten · 2017-11-13T20:43:43Z

I've added a couple of comments, hopefully it's a bit clearer what the code is trying to do!

Zetten · 2017-12-29T16:03:34Z

Thanks for merging - I'd forgotten about this!

Just FYI I did start looking at how to use the new runfiles features from Bazel 0.8.0 but got a bit sidetracked. I didn't test all the different ways a node_binary can be called, but it should be possible to simplify a couple of the cases at least. I'll try to knock something into shape for a PR in the new year.

pcj · 2017-12-29T16:06:15Z

Sounds good!

fkorotkov · 2018-02-05T19:44:39Z

@Zetten @pcj It seems these tests are failing now even though the latest master build is green. The failure is:

INFO: Build completed, 1 test FAILED, 23523 total actions
//:polymer-cli_bin_test                                                  FAILED in 14.4s

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
-----------------------------------------------------------------------------
module.js:472
    throw err;
    ^

Error: Cannot find module 'lazy-req'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/private/var/tmp/_bazel_fedor/3cf928c1983148e1d64fee3f3cf58fdd/execroot/org_pubref_rules_node_polymer_test/bazel-out/darwin-fastbuild/bin/external/yarn_modules/polymer-cli_polymer_bin_files/node_modules/polymer-cli/node_modules/update-notifier/index.js:5:19)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)

I can reproduce it locally and in my fork on Travis CI.

Zetten · 2018-02-05T19:54:35Z

Yeah, I saw this pop up in my own project recently as well. Somewhere along the way there are two different transitive versions of update-notifier getting pulled in. I think the 2.x line has removed lazy-req as a dependency, so it's missed out across the board, even though update-notifier 1.x is still there and using it.

I didn't dig into the npm/yarn dependency stuff too much (that way madness lies...) but I guess the solution is to fix the version of update-notifier (and possibly other deps too) in the resolutions attribute of the yarn_modules target.

Unfortunately my project has decided to no longer use polymer (and polymer-cli) and I haven't been able to devote any time to figuring out the issue properly.

Zetten · 2018-02-05T20:08:49Z

I've offered a short workaround in #57. If it recurs I would suggest pinning all transitive dependency versions as a solution.

Zetten mentioned this pull request Nov 1, 2017

Build fails when using the protobuf.js library #45

Open

Zetten force-pushed the PolymerCli branch from c44779e to f011f3c Compare November 1, 2017 12:11

Zetten force-pushed the PolymerCli branch from f011f3c to ca36349 Compare November 6, 2017 13:28

Add tests for polymer-cli

5de1910

Zetten force-pushed the PolymerCli branch from ca36349 to 2aa971b Compare November 6, 2017 15:40

pcj requested changes Nov 8, 2017

View reviewed changes

pcj reviewed Nov 8, 2017

View reviewed changes

Zetten force-pushed the PolymerCli branch from 2aa971b to dbbec01 Compare November 13, 2017 20:41

Zetten added 5 commits November 17, 2017 20:26

Omit filenames with spaces from node_modules

d6e42d5

Collect @foo/bar modules from node_modules

e7ba80e

Set node_modules dir correctly for @foo/bar modules

e51015e

Remove only circular dependencies from a cycle, not all dependencies

2c7dc59

Add named-workspace-relative runfiles lookup to node_binary

a57ab23

Zetten force-pushed the PolymerCli branch from dbbec01 to a57ab23 Compare November 17, 2017 20:27

pcj approved these changes Dec 29, 2017

View reviewed changes

pcj merged commit 277a65c into pubref:master Dec 29, 2017

Zetten deleted the PolymerCli branch December 29, 2017 16:03

Zetten mentioned this pull request Feb 5, 2018

Fix polymer-cli test by specifying update-notifier version #57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for polymer-cli binary #48

Add tests for polymer-cli binary #48

Zetten commented Nov 1, 2017

Zetten commented Nov 1, 2017

pcj commented Nov 1, 2017

Zetten commented Nov 6, 2017

pcj commented Nov 6, 2017 via email

Zetten commented Nov 8, 2017

pcj left a comment

pcj Nov 8, 2017

Zetten Nov 8, 2017

pcj Nov 8, 2017 •

edited

Loading

Zetten Nov 8, 2017 •

edited

Loading

Zetten commented Nov 13, 2017

Zetten commented Dec 29, 2017

pcj commented Dec 29, 2017

fkorotkov commented Feb 5, 2018

Zetten commented Feb 5, 2018

Zetten commented Feb 5, 2018

Add tests for polymer-cli binary #48

Add tests for polymer-cli binary #48

Conversation

Zetten commented Nov 1, 2017

Zetten commented Nov 1, 2017

pcj commented Nov 1, 2017

Zetten commented Nov 6, 2017

pcj commented Nov 6, 2017 via email

Zetten commented Nov 8, 2017

pcj left a comment

Choose a reason for hiding this comment

pcj Nov 8, 2017

Choose a reason for hiding this comment

Zetten Nov 8, 2017

Choose a reason for hiding this comment

pcj Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Zetten Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Zetten commented Nov 13, 2017

Zetten commented Dec 29, 2017

pcj commented Dec 29, 2017

fkorotkov commented Feb 5, 2018

Zetten commented Feb 5, 2018

Zetten commented Feb 5, 2018

pcj Nov 8, 2017 •

edited

Loading

Zetten Nov 8, 2017 •

edited

Loading