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

prepare for breaking change in rules_nodejs #126

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

alexeagle
Copy link
Contributor

bazel-contrib/rules_nodejs#2125 is going to turn off the patching of the require() function by default. The reason is that it breaks compatibility with many node programs. However rules_sass depends on this, and also has at least conceptual sharing with google3 which uses the patched require() as well.

This change is a no-op for rules_nodejs 2.x where the default is true, but is required with rules_nodejs 3.0 where it will be false.

@nex3
Copy link
Collaborator

nex3 commented Dec 4, 2020

Can we avoid using the patched require()? We don't use Node at all in google3, so compatibility with that shouldn't be an issue.

bazel-contrib/rules_nodejs#2125 is going to turn off the patching of the require() function by default. The reason is that it breaks compatibility with many node programs. However rules_sass depends on this, and also has at least conceptual sharing with google3 which uses the patched require() as well.

This change is a no-op for rules_nodejs 2.x where the default is true, but is required with rules_nodejs 3.0 where it will be false.
@alexeagle
Copy link
Contributor Author

Sure, you could do that. In code I own I'm mostly removing the need for patching it.

Generally the purpose of that patching in google3 was to make all programs inherently "runfiles-aware", that is, to provide the "unionfs" view of the filesystem as if the input tree and the output tree were overlaid. To remove the need for patching, you'll have to find places where you rely on this, and instead either

  • explicitly use a Runfiles helper library to resolve paths, or
  • make the program run exclusively using files in the input tree or exclusively using files in the output tree, with an appropriate current working directory.

@alexeagle
Copy link
Contributor Author

I'm guessing your team doesn't have much time to spend on OSS so I'd suggest accepting this PR to preserve the status quo, then remove the need for the patches when you find time.

@nex3 nex3 merged commit 931b54a into bazelbuild:master Dec 4, 2020
@nex3
Copy link
Collaborator

nex3 commented Dec 4, 2020

Can you file an issue with details on how to fix this going forward?

@alexeagle
Copy link
Contributor Author

Thanks, I appreciate you merging it!

I'm not sure exactly where the fix belongs, mostly because the symptom I observed was that the sass compiler was just hanging. I probably could have connected a remote debugger to it so I could see the stack - but maybe it would have been hard to decipher since it's produced from the dart2js codegen.

You can repro just by flipping the flag

diff --git a/sass/BUILD b/sass/BUILD
index 567828a..d46ce02 100644
--- a/sass/BUILD
+++ b/sass/BUILD
@@ -20,6 +20,6 @@ nodejs_binary(
     # rules_nodejs 3.0 will flip the default for this flag which breaks rules_sass users
     templated_args = [
         "--nobazel_node_patches",
-        "--bazel_patch_module_resolver",
+        "--nobazel_patch_module_resolver",
     ],
 )

and then bazel test //...
Then I see these hanging:

    SassCompiler examples/hello_world/main-sourcemap-embed-sources.css; 9s worker
    SassCompiler examples/hello_world/main-no-sourcemap.css; 9s worker
    SassCompiler examples/imports_css/main.css; 9s worker
    SassCompiler examples/hello_world/main.css; 9s worker

@nex3
Copy link
Collaborator

nex3 commented Dec 9, 2020

What does the patch do specifically?

@alexeagle
Copy link
Contributor Author

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.

2 participants