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

Ability to stop propagation of link_node_modules aspect information #2941

Closed
devversion opened this issue Sep 13, 2021 · 2 comments
Closed

Comments

@devversion
Copy link
Contributor

devversion commented Sep 13, 2021

The link_node_modules aspect is currently used for capturing mappings that the linker can consume. The aspect is applied on the target relying on the linker (such as a nodejs_test). It would be beneficial to have an API that allows for a target to skip the module mapping collection. This is useful if a target applies an outgoing transition and therefore results in transitive dependencies being built under a different output directory (compared to bazel-out).

This is problematic because a test target for example cannot rely on the target applying the transition (e.g. a pkg_npm target), as well as on the non-transitioned original dependency (such as the ts_library used for the NPM package). e.g.

Error in fail: conflicting mapping at 'test': ':core'
  and ':core' map to conflicting 
   bazel-out/x64_windows-fastbuild/bin/packages/core and
   bazel-out/x64_windows-fastbuild-ST-2e5f3376adb5/bin/packages/core

As it can be observed above: There are two conflicting mappings because core dependency on the
pkg_npm_wrapped target is also considered as linker mapping for the test. This is unexpected and it
would be fixable if there was a way for the pkg_npm_wrapped/ng_package rule to return an empty
set of mappings / or disable the propagation of the e.g. deps attribute. The mappings from
the transitive dependencies are not desirable anyway because the NPM package is supposed to be the
mapping if at all.

ts_libary(
  name = "core",
  deps = [..],
  package_name = "@angular/core",
)

# This is a wrapper-implementation of `pkg_npm` that has an outgoing transition
# applied on "deps". The ":core" target will be built with different build settings.
pkg_npm_wrapped(
  name = "core_npm_package",
  deps = [":core"],
)

######

ts_library(
  name = "test_lib",
  deps = [":core"],
)

jasmine_node_test(
  name = "test",
  data = ["core_npm_package"],
  deps = [":test_lib"]
)

Workaround tried:

I have tried working around it by manually setting the link_node_modules__aspect_result property on the NPM package
rule. e.g.

    return struct(**{
        "files": depset([package_dir]),
        "link_node_modules__aspect_result": {}
    })

This does not work because the link_node_modules aspect does not check if the property is already set and Bazel will error because an already-existing "provider" is re-assigned. A solution would be to check for the presence in the aspect before. This will allow for advanced cases like described.

Ideally the link_node_modules aspect should not use the deprecated? struct approach for providers to begin with. Then it should check for the presence of the provider before trying to set it in the aspect (allowing for a solution like in the snippet above). This will allow for advanced use-cases. Alternatively, the aspect could look for an attribute on the target it visits. Though I like the idea of having manual control over mappings.

I'm happy to send a PR. Might send a proposal tomorrow.

devversion added a commit to devversion/rules_nodejs that referenced this issue Sep 15, 2021
…ules` aspect

This commit does two things:

  1. It switches the `link_node_modules` aspect from the deprecated
     struct provider variant to the provider list return value.
  2. The aspect no longer collects aspect results from dependencies
     if the target already provides explicit mapping information for the
     linker. This is an API for advanced use-cases as outlined in
     bazel-contrib#2941.
devversion added a commit to devversion/rules_nodejs that referenced this issue Sep 15, 2021
…ules` aspect

This commit does two things:

  1. It switches the `link_node_modules` aspect from the deprecated
     struct provider variant to the provider list return value.
  2. The aspect no longer collects aspect results from dependencies
     if the target already provides explicit mapping information for the
     linker. This is an API for advanced use-cases as outlined in
     bazel-contrib#2941.
alexeagle pushed a commit that referenced this issue Sep 17, 2021
…ules` aspect

This commit does two things:

  1. It switches the `link_node_modules` aspect from the deprecated
     struct provider variant to the provider list return value.
  2. The aspect no longer collects aspect results from dependencies
     if the target already provides explicit mapping information for the
     linker. This is an API for advanced use-cases as outlined in
     #2941.
@alexeagle
Copy link
Collaborator

fixed by #2947

@devversion
Copy link
Contributor Author

thanks @alexeagle! 🎉

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

No branches or pull requests

2 participants