fix: support running with --enable_runfiles on Windows #2258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dae
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
|
Looks like this has broken all the Windows tests - I'll try dig into it some more. |
I was finding the run and test commands on a nodejs binary on Windows gave an error message similar to the one mentioned in the following post when --enable_runfiles is passed in on the command line: bazel-contrib#2178 (comment) Running with --noenable_runfiles, the tests worked correctly, but other packages in my repo require runfiles. This patch changes the resolution to use the MANIFEST file inside the runfiles folder when runfiles are enabled, which seems to fix the issue for me.
|
I'm afraid I was not even able to get this repo building on Windows to try the tests locally - it errors out just trying to build @nodejs//:yarn_node_repositories on my Windows system. I tested the original fix by building a release.tar.gz on a Mac machine and copying it over to a Windows system.. |
|
Looks like the test assertions are easy enough to update even without having a windows machine available. I'll see if @gregmagolan can take a look since he authored the test that needs to change |
|
Yep, looks like it does - thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was finding the run and test commands on a 2.2.2 nodejs binary on
Windows gave an error message similar to the one mentioned in the
following post when --enable_runfiles is passed in on the command line:
#2178 (comment)
Running without runfiles, my nodejs tests worked correctly, but other packages
in my repo require runfiles, and switching the setting forces many things to be
recompiled.
This patch changes the resolution to use the MANIFEST file inside
the runfiles folder when runfiles are enabled, which seems to fix
the issue.
I'm new to the codebase, and only use it in a limited fashion at the moment,
so this change could benefit from a review by someone more familiar with the code.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information