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(runfiles): use normalized paths when guarding runfiles root and node_modules on Windows #3331

Merged

Conversation

mistic
Copy link
Contributor

@mistic mistic commented Feb 16, 2022

PR Checklist

Please check if your PR fulfils the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behaviour?

After #3060 being merged in order to fix a linker problem the paths we collecte to guard the runfiles and the runfiles node_modules at https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/node/launcher.sh#L237 are crashing the node resolution when running on Windows as it is using non normalized paths.

What is the new behaviour?

That PR makes sure we normalize the collected paths on Windows so that we can later build a correct BAZEL_PATCH_ROOTS.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Closes #3054 and fixes the build node_modules resolutions when on Windows.

@alexeagle
Copy link
Collaborator

Windows CI Failure is already present on stable, not introduced here...

@mistic mistic marked this pull request as ready for review February 16, 2022 22:52
@mistic mistic force-pushed the fix-broken-runfiles-path-on-windows branch from af06993 to f80bacb Compare February 22, 2022 22:03
@gregmagolan
Copy link
Collaborator

Windows CI Failure is already present on stable, not introduced here...

These failures @alexeagle



(22:29:51) ERROR: C:/b/bk-windows-p9mw/bazel/rules-nodejs-nodejs/internal/node/test/chdir/BUILD.bazel:24:16: output 'internal/node/test/chdir/package.json' was not created
--
  | (22:29:51) ERROR: C:/b/bk-windows-p9mw/bazel/rules-nodejs-nodejs/internal/node/test/chdir/BUILD.bazel:24:16: Action internal/node/test/chdir/package.json failed: not all outputs were created or valid


@mistic
Copy link
Contributor Author

mistic commented Feb 28, 2022

@alexeagle can we get this one in? 😃

@alexeagle
Copy link
Collaborator

oops sorry it missed today's release!! we finally greened up HEAD again...

@alexeagle alexeagle merged commit 7993296 into bazel-contrib:stable Mar 1, 2022
@mistic
Copy link
Contributor Author

mistic commented Apr 13, 2022

@alexeagle do you think we can backport this into 4.x? I think it is important to have that fix there too.

@alexeagle
Copy link
Collaborator

sure can you send a PR against the 4.x branch? I'll merge and release a patch

mistic added a commit to mistic/rules_nodejs that referenced this pull request Apr 13, 2022
…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

I've opened the PR 👍

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

change in run_node behaviour from 4.4.1 to 4.4.2
3 participants