[Zetten]

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]

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]

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

[Zetten]

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]

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]

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]

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?

[Zetten]

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]

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.

[Zetten]

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]

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

[Zetten]

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]

Sounds good!

[fkorotkov]

@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]

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]

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