Add support for $(location) in nodejs_binary#entry_point #42
Conversation
alexeagle
force-pushed
the
issue_32
branch
3 times, most recently
from
edf8117 to
e1b50ef
Compare
|
@vladmos I would appreciate an opinion from the Bazel team: I'm using |
alexeagle
force-pushed
the
issue_32
branch
4 times, most recently
from
6f10738 to
cdb2a1b
Compare
|
@rzhw any comments? I'm still a bit unsure if users should be exposed to the |
examples/program/BUILD.bazel
Outdated
| py_test( | ||
| name = "bin_test", | ||
| srcs = ["bin_test.py"], | ||
| deps = ["@build_bazel_rules_nodejs//internal:runfiles"], |
There was a problem hiding this comment.
Nit: Does using just //internal:runfiles work, since this is just an example that lives in this workspace?
There was a problem hiding this comment.
Whoops, didn't realise this example was a separate workspace.
There was a problem hiding this comment.
Yeah having two workspaces in this repo is useful for learning about things like this, and also catches some bugs where the wrong context is used to create a path.
|
bazelbuild/bazel#2475 looks relevant, particularly this proposal. |
|
Awesome job tracking down that feature request. It looks like Ulf is close to introducing the |
|
Looks like Tests here, doesn't appear to test for e.g. |
|
What's currently blocking this from landing? I've tried rebasing once but looks like I'm hitting some conflicts again 😢 |
|
Nothing blocking this other than prioritization. I haven't had time, would appreciate some help! (Also I'm switching to Windows to force myself to make all the Bazel + Angular builds work on Windows so it's going to be slow going for a while....) |
|
Gotcha, what's needed after resolving the merge conflicts if any? Verifying it works on Windows, design input from someone else from Bazel? |
|
Sorry @rzhw just never prioritized this one. Dusting it off now... |
BREAKING CHANGE: This forces us to use workspace-relative paths since the rootpath helper returns this format. Users are now exposed to the external path segment to reference other workspaces. See discussion: bazel-contrib#84 Fixes bazel-contrib#32
|
@alexeagle Any reason this can't be merged? I could help out if it's a time thing. It is confusing for others in my team who don't know the details of the rules_nodejs implementation to understand what this field should be. Using a build target instead hides the implementation details better. |
|
@nimerrit This has been collecting some dust. I've been looking at related code recently with #240. I'll discuss this PR with @alexeagle when he's back from vacation next week and to see what else needs to be done so that it can get in. It would be a good one to land for users like yourself. |
| return path | ||
| return expand_path_into_runfiles(ctx, path) | ||
|
|
||
| def expand_path_into_runfiles(ctx, path): |
There was a problem hiding this comment.
expand_path_into_runfiles is used in rules_typescript so likely need to keep it around as deprecated
| // or just path/to/thing with the implied local workspace | ||
| // This matches Bazel semantics for cwd of toolchain processes | ||
| // and helpers like ctx.expand_location("$(rootpath foo)") | ||
| resolveRunfiles('TEMPLATED_user_workspace_name', request), |
There was a problem hiding this comment.
This seems make sense. It should also continue to work with #240.
|
@alexeagle @gregmagolan Any updates on this? I'm trying to calculate the entry_point in a macro and can't figure out how to do it currently. The discussion in #32 would have me believe that I can use |
|
Given a discussion here: bazelbuild/bazel#6001 (and updated docs: bazelbuild/bazel@22a4994) it seems that entry_point should not be a string and not use the |
|
Will go with a label, thanks @Globegitter |
Fixes bazel-contrib#41 Closes bazel-contrib#42 PiperOrigin-RevId: 169476478
Fixes bazel-contrib#41 Closes bazel-contrib#42 PiperOrigin-RevId: 169476478
No description provided.