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.

Sign up for GitHub

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

Jump to bottom

Module Mapping Regression (conflicting mapping at ...) #1595

Closed
joeljeske opened this issue Jan 31, 2020 · 3 comments · Fixed by #1597
Closed

Module Mapping Regression (conflicting mapping at ...) #1595

joeljeske opened this issue Jan 31, 2020 · 3 comments · Fixed by #1597

Comments

@joeljeske
Copy link
Contributor

🐞 bug report

Affected Rule

The issue is caused by the rule: nodejs_binary

Is this a regression?

Yes, the previous version in which this bug was not present was: 1.1.0

Description

I believe the bug is incorrect boolean logic when deciding to use a module mapping or not.

I believe the issue is in this section here:

110e00e#diff-3238816daaca23ffcb6861ceb46dfc2aR44-R47

Perhaps the change needed is this?

           if mappings[k][0] == "runfiles":
                return True
-         elif v[0] != "runfiles":
+         elif v[0] == "runfiles":
               return False

🔬 Minimal Reproduction

I'm not entirely sure how to reproduce this, as it occurs in a setup with a lot of surrounding rules, however I suspect that @gregmagolan might be able to help me with reproduction as he seemed to be working on this issue very recently.

🔥 Exception or Error


conflicting mapping at //some:target: some/module-name maps to both ["bin", "ws/some/module-name"] and ["runfiles", "ws/some/module-name"]

🌍 Your Environment

Operating System:

  
macOS 10.14.6

Output of bazel version:

  
Build label: 1.2.0
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Wed Nov 20 15:08:12 2019 (1574262492)
Build timestamp: 1574262492
Build timestamp as int: 1574262492

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
v1.2.2

Anything else relevant?
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Jan 31, 2020
@gregmagolan
fix: fix logic error in linker conflict resolution
fe72f51 
Fixes bazel-contrib#1595. The `elif` path in
```
            if mappings[k][0] == "runfiles":
                return True
            elif v[0] == "runfiles":
                return False
```
is now exercised by the linker integration tests.
@gregmagolan gregmagolan mentioned this issue Jan 31, 2020
Merged
12 tasks
gregmagolan added a commit to gregmagolan/rules_nodejs that referenced this issue Jan 31, 2020
@gregmagolan
fix(builtin): fix logic error in linker conflict resolution
f30d290 
Fixes bazel-contrib#1595. The `elif` path in
```
            if mappings[k][0] == "runfiles":
                return True
            elif v[0] == "runfiles":
                return False
```
is now exercised by the linker integration tests.
@gregmagolan
Copy link
Collaborator

Thanks @joeljeske . You are correct that the boolean is backwards and the integration test didn't exercise that path. #1597 will fix it and I'll cut another patch release.

@joeljeske
Copy link
Contributor Author

Awesome thanks for quick feedback!

gregmagolan added a commit that referenced this issue Jan 31, 2020
@gregmagolan
fix(builtin): fix logic error in linker conflict resolution (#1597)
b864223 
Fixes #1595. The `elif` path in
```
            if mappings[k][0] == "runfiles":
                return True
            elif v[0] == "runfiles":
                return False
```
is now exercised by the linker integration tests.
@gregmagolan
Copy link
Collaborator

Np. Pushed a quick 1.2.4 release that includes the fix since it fixes the 1.2.0 regression. https://github.com/bazelbuild/rules_nodejs/releases/tag/1.2.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(builtin): fix logic error in linker conflict resolution gregmagolan/rules_nodejs
2 participants
@gregmagolan @joeljeske