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

Hook up linker to nodejs_binary/nodejs_test #1430

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

alexeagle
Copy link
Collaborator

For #1382
Pairing with @mrmeku on it today

@alexeagle alexeagle force-pushed the link_all_the_things branch 5 times, most recently from 98bb420 to 861d445 Compare December 6, 2019 23:46
@alexeagle alexeagle marked this pull request as ready for review December 6, 2019 23:50
@alexeagle alexeagle force-pushed the link_all_the_things branch 2 times, most recently from 57b5cc1 to b697944 Compare December 7, 2019 04:00
@@ -18,6 +18,6 @@
},
"scripts": {
"test": "bazel test ...",
"postinstall": "patch-package --patch-dir=./tools/patches"
"// postinstall": "patch-package --patch-dir=./tools/patches"
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove entirely?

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

@googlebot googlebot added cla: no and removed cla: yes labels Dec 7, 2019
@gregmagolan gregmagolan force-pushed the link_all_the_things branch 2 times, most recently from 7ba210a to 96e99e1 Compare December 8, 2019 03:37
@mrmeku
Copy link
Collaborator

mrmeku commented Dec 9, 2019

@alexeagle Looks like the only failing test was already failing in prior builds: https://buildkite.com/bazel/rules-nodejs-nodejs/builds/5046

It also passes on my personal mac :)

@gregmagolan
Copy link
Collaborator

Ahh. Thanks for pointing that out. It's a strange heisenbug that shows up only on bazelci osx. I haven't been able to reproduce it locally either.

@mrmeku
Copy link
Collaborator

mrmeku commented Dec 9, 2019

@gregmagolan The package the error references is jest-haste-map so maybe try to lock down the version for that dependency even more by using the resolutions section of package.json: https://yarnpkg.com/lang/en/docs/selective-version-resolutions/

And if that doesn't work just pray to your chosen deity and ask for forgiveness

This makes it easier to reuse in nodejs_binary
- nodejs_binary and nodejs_test will always produce a modules manifest needed by the linker
- if the program isn't run with a user-specific linker manifest, we use the static one as a default
- always pass the flag to opt-out of custom module resolver for generated rules

Fixes bazel-contrib#1382
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@gregmagolan gregmagolan self-requested a review December 10, 2019 15:57
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 3321ed5 into bazel-contrib:master Dec 10, 2019
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.

4 participants