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

[4.x] fix(runfiles): use normalized paths when guarding runfiles root and node_modules on Windows #3409

Merged

Conversation

mistic
Copy link
Contributor

@mistic mistic commented Apr 13, 2022

That PR is a backport of #3331 into 4.x.

…ode_modules on Windows (bazel-contrib#3331)

* fix(runfiles): use normalized paths when guarding runfiles root and node_modules on Windows

* refactor(builtin): run new code path only on Windows
@mistic
Copy link
Contributor Author

mistic commented Apr 13, 2022

Just tested to see if the CI problem had anything to do with my changes and it has not. At 8790485 I've commented out all the changes that I made and recover the original line and the failures were the same in the Build Run.

Reverting those now so we can move forward with the PR

@mistic
Copy link
Contributor Author

mistic commented Apr 19, 2022

@alexeagle can we get this merged in? 😎

@alexeagle
Copy link
Collaborator

I was confused by your "Reverting those now..." comment, and whether this is ready. Can you be more clear what is the state of the 4.x branch? Are you saying it's already red at HEAD? https://buildkite.com/bazel/rules-nodejs-nodejs/builds?branch=4.x is green so something seems inconsistent.

@mistic
Copy link
Contributor Author

mistic commented Apr 20, 2022

The CI job currently failing doesn't seemed to have nothing to do with that PR however you retried it a couple of times and it continued to fail. In order to validate that was not related to my changes I've commented those at 8790485, then the CI failed again at https://buildkite.com/bazel/rules-nodejs-nodejs/builds/11933 with the same errors so I was sure it was not related to the changes on this PR and to complete it I've reverted my changes to test the CI at 7fa864c.

I'm not sure what changed in between the latest commit that made into 4.x but I don't think the CI was either green at the time it was merged #3343 (comment).

I think the reasons why 4.x ci is broken is definitely not related with that PR and we can move it forward and then look to fix the CI itself in an independent PR.

@mistic
Copy link
Contributor Author

mistic commented Apr 20, 2022

What I wrote above is also validated by the manual run you did at https://buildkite.com/bazel/rules-nodejs-nodejs/builds/11963

@alexeagle
Copy link
Collaborator

Yes, I understood that you were hitting an existing failure. I am surprised we are not running buildkite on the 4.x branch, and I had to manually trigger to discover that it's broken.
I'd rather get it green first before accepting more contributions on that branch. "leave it broken" isn't a good policy...

@alexeagle
Copy link
Collaborator

#3418 to get CI green again

@alexeagle
Copy link
Collaborator

Still red, that's what I expected since your PR had more tests failing than HEAD did...

@mistic
Copy link
Contributor Author

mistic commented Apr 20, 2022

Alright, had the need to cherry-pick a fix for that specific test also introduced on stable for the same purpose. CI is now green again

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

thanks for getting it green!

@alexeagle alexeagle merged commit 8f3c98d into bazel-contrib:4.x Apr 25, 2022
alexeagle pushed a commit that referenced this pull request Apr 25, 2022
… and node_modules on Windows (#3409)

* fix(runfiles): use normalized paths when guarding runfiles root and node_modules on Windows (#3331)

* fix(runfiles): use normalized paths when guarding runfiles root and node_modules on Windows

* refactor(builtin): run new code path only on Windows

* chore(builtin): test ci

* Revert "chore(builtin): test ci"

This reverts commit 8790485.

* chore(builtin): cherry-pick 8606c50

* docs(builtin): do not change unnecessary docs for this PR purpose

Co-authored-by: Paul Gschwendtner <paulgschwendtner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants