Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(builtin): look in the execroot for nodejs_binary source entry_points #1816

Merged
merged 1 commit into from
Apr 11, 2020

Conversation

alexeagle
Copy link
Collaborator

In a6e29c2 we added support for generated entry_point by adding a secondary lookup.
In 863c7de we reversed the order of the lookups to make rollup work in a certain use case.
Neither of these was principled, because we know ahead of time whether the entry_point is generated. We can use a single lookup.

This also makes sure that programs run in the location where the linker will put them (note that the logic in question runs before the linker has created the node_modules directory in the execroot.)
This is why #1787 observes that some node programs were broken - we were running the entry point as
bazel-out/host/bin/external/npm/@graphql-codegen/cli/bin/graphql-codegen.sh.runfiles/npm/node_modules/@graphql-codegen/cli/bin.js
when it should have been
node_modules/@graphql-codegen/cli/bin.js

Fixes #1787

@alexeagle alexeagle force-pushed the fix1787 branch 2 times, most recently from 244eb24 to f81e946 Compare April 10, 2020 23:11
@alexeagle
Copy link
Collaborator Author

This is broken by targets which rely on a generated bin because those are still not getting linked.

We need #1455 for this to pass

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

In a6e29c2 we added support for generated entry_point by adding a secondary lookup.
In 863c7de we reversed the order of the lookups to make rollup work in a certain use case.
Neither of these was principled, because we know ahead of time whether the entry_point is generated. We can use a single lookup.
However that depends on running the linker on all generated bin rules, which is a breaking change, so for now we just defer the FS check until after the linker runs.

This also makes sure that programs run in the location where the linker will put them (note that the logic in question runs before the linker has created the node_modules directory in the execroot.)
This is why bazel-contrib#1787 observes that some node programs were broken - we were running the entry point as
bazel-out/host/bin/external/npm/@graphql-codegen/cli/bin/graphql-codegen.sh.runfiles/npm/node_modules/@graphql-codegen/cli/bin.js
when it should have been
node_modules/@graphql-codegen/cli/bin.js

Fixes bazel-contrib#1787
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Collaborator

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

@gregmagolan gregmagolan merged commit b84d65e into bazel-contrib:master Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in 1.5.0 generated rules
3 participants