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

[Bug]: Dependency aliases of transitive dependencies are not working #1062

Open
devversion opened this issue May 12, 2023 · 10 comments
Open
Labels
bug Something isn't working funding needed Contribute to https://opencollective.com/aspect-build

Comments

@devversion
Copy link
Contributor

What happened?

Consider you are using a project that has a direct dependency on A. Where A then has a dependency on B, but the package is aliased to e.g. alias. e.g.

  registry.npmjs.org/@isaacs/cliui/8.0.2:
    name: '@isaacs/cliui'
    version: 8.0.2
    dependencies:
      string-width-cjs: registry.npmjs.org/string-width/4.2.3

rules_js will properly configure the store target for cliui to look for string-width-cjs as a store dependency. But the store target with the aliased name does actually not exist.

This causes errors like:

ERROR: /usr/local/google/home/pgschwendtner/projects/material.angular.io/BUILD.bazel:9:22: in deps attribute
of npm_package_store_internal rule
//:.aspect_rules_js/node_modules/@[email protected]+@isaacs+cliui+8.0.2/pkg: \
target '//:.aspect_rules_js/node_modules/[email protected]+string-width+4.2.3/ref' does
not exist. Since this rule was created by the macro 'npm_link_all_packages', the error might have been caused by the macro implementation

Version

Linux amd64. Rules_JS rules_js-1.20.1. Bazel 5.1.0. A team member also tried with the latest version.

How to reproduce

No response

Any other information?

Looking into it without diving too much into implementation, the issue seems to be that pnpm_lock_file.packages() is used to create the store entries. But this never holds the aliased package names. i.e. rules_js iterates through the packages: entry in the lock file and then creates store entries for those.

There will never be an entry with the alias though because the alias name will only ever appear in the dependencies section of an entry. The transitive closure detection already has access to this information.

Likely we could use that to compile a more precise list of packages that will then be used
to create the links.

@devversion devversion added the bug Something isn't working label May 12, 2023
@github-actions github-actions bot added the untriaged Requires traige label May 12, 2023
@gregmagolan
Copy link
Member

Have a repro here, #1063. Working on a fix.

@gregmagolan
Copy link
Member

gregmagolan commented May 13, 2023

Hmm. It is a tricky one and more than an hour of work. I think this one will have to wait until next week when either @jbedard or I have more time.

@gregmagolan gregmagolan removed the untriaged Requires traige label May 13, 2023
@gregmagolan gregmagolan moved this to 🏗 In progress in Open Source May 13, 2023
@gregmagolan
Copy link
Member

gregmagolan commented May 13, 2023

Notes from repro in #1063...

In /private/var/tmp/_bazel_greg/7aed427a991d86f99332ab79b9b11780/external/npm/repositories.bzl the generated npm_import for the problematic @isaacs/[email protected] package looks like this,

    npm_import(
        name = "npm__at_isaacs_cliui__registry.npmjs.org_at_isaacs_cliui_8.0.2",
        root_package = "",
        link_workspace = "",
        link_packages = {
            "npm/private/test": ["@isaacs/cliui"],
        },
        package = "@isaacs/cliui",
        version = "registry.npmjs.org/@isaacs/[email protected]",
        url = "https://registry.yarnpkg.com/@isaacs/cliui/-/cliui-8.0.2.tgz",
        npm_translate_lock_repo = "npm",
        dev = True,
        generate_bzl_library_targets = True,
        integrity = "sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==",
        deps = {
            "string-width": "registry.npmjs.org/[email protected]",
            "string-width-cjs": "registry.npmjs.org/[email protected]",
            "strip-ansi": "registry.npmjs.org/[email protected]",
            "strip-ansi-cjs": "registry.npmjs.org/[email protected]",
            "wrap-ansi": "registry.npmjs.org/[email protected]",
            "wrap-ansi-cjs": "registry.npmjs.org/[email protected]",
        },
        transitive_closure = {
            "@isaacs/cliui": ["registry.npmjs.org/@isaacs/[email protected]"],
            "string-width": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "string-width-cjs": ["registry.npmjs.org/[email protected]"],
            "strip-ansi": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "strip-ansi-cjs": ["registry.npmjs.org/[email protected]"],
            "wrap-ansi": ["registry.npmjs.org/[email protected]"],
            "wrap-ansi-cjs": ["registry.npmjs.org/[email protected]"],
            "ansi-styles": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "ansi-regex": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "emoji-regex": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "is-fullwidth-code-point": ["registry.npmjs.org/[email protected]"],
            "color-convert": ["registry.npmjs.org/[email protected]"],
            "color-name": ["registry.npmjs.org/[email protected]"],
            "eastasianwidth": ["registry.npmjs.org/[email protected]"],
        },
    )

The transitive closure is calculated wrong from the looks of it, it should likely be:

        transitive_closure = {
            "@isaacs/cliui": ["registry.npmjs.org/@isaacs/[email protected]"],
            "string-width": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "strip-ansi": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "wrap-ansi": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "ansi-styles": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "ansi-regex": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "emoji-regex": ["registry.npmjs.org/[email protected]", "registry.npmjs.org/[email protected]"],
            "is-fullwidth-code-point": ["registry.npmjs.org/[email protected]"],
            "color-convert": ["registry.npmjs.org/[email protected]"],
            "color-name": ["registry.npmjs.org/[email protected]"],
            "eastasianwidth": ["registry.npmjs.org/[email protected]"],
        },

Although even then I think there will also be fixed needed in npm_import itself that generates links the /private/var/tmp/_bazel_greg/7aed427a991d86f99332ab79b9b11780/external/npm__at_isaacs_cliui__registry.npmjs.org_at_isaacs_cliui_8.0.2__links/defs.bzl for the package. In particular, the deps, lc_deps & ref_deps there which currently look like

    deps = {
            ":.aspect_rules_js/{}/@[email protected]+@[email protected]/pkg".format(link_root_name): "@isaacs/cliui",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "is-fullwidth-code-point",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-convert",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-name",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "eastasianwidth",
        }
    lc_deps = {
            ":.aspect_rules_js/{}/@[email protected]+@[email protected]/pkg_pre_lc_lite".format(link_root_name): "@isaacs/cliui",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "is-fullwidth-code-point",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-convert",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-name",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "eastasianwidth",
        }
    ref_deps = {
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "string-width-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "strip-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "wrap-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "wrap-ansi-cjs",
        }

@gregmagolan
Copy link
Member

cc @jbedard

@gregmagolan
Copy link
Member

gregmagolan commented May 13, 2023

The other key observation is that this issue ONLY happens when the package name in the pnpm lock file starts with registry.npmjs.org/ such as

  registry.npmjs.org/@isaacs/[email protected]:
    resolution: {integrity: sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==, registry: https://registry.yarnpkg.com/, tarball: https://registry.npmjs.org/@isaacs/cliui/-/cliui-8.0.2.tgz}
    name: '@isaacs/cliui'
    version: 8.0.2
    engines: {node: '>=12'}
    dependencies:
      string-width: registry.npmjs.org/[email protected]
      string-width-cjs: registry.npmjs.org/[email protected]
      strip-ansi: registry.npmjs.org/[email protected]
      strip-ansi-cjs: registry.npmjs.org/[email protected]
      wrap-ansi: registry.npmjs.org/[email protected]
      wrap-ansi-cjs: registry.npmjs.org/[email protected]
    dev: true

which only happens when the .npmrc registry setting is set for the package in the repro. In the repro it is set to registry=https://registry.yarnpkg.com.

When registry is not set for the package in npmrc then the pnpm-lock.yaml file package entry looks like this:

  /@isaacs/[email protected]:
    resolution: {integrity: sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==}
    engines: {node: '>=12'}
    dependencies:
      string-width: 5.1.2
      string-width-cjs: /[email protected]
      strip-ansi: 7.0.1
      strip-ansi-cjs: /[email protected]
      wrap-ansi: 8.1.0
      wrap-ansi-cjs: /[email protected]
    dev: true

with the aliases clearly distinguishable and rules_js correctly handles this default case.

The generated npm_import looks like this in this case,

    npm_import(
        name = "npm__at_isaacs_cliui__8.0.2",
        root_package = "",
        link_workspace = "",
        link_packages = {
            "npm/private/test": ["@isaacs/cliui"],
        },
        package = "@isaacs/cliui",
        version = "8.0.2",
        url = "https://registry.npmjs.org/@isaacs/cliui/-/cliui-8.0.2.tgz",
        npm_translate_lock_repo = "npm",
        dev = True,
        generate_bzl_library_targets = True,
        integrity = "sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==",
        deps = {
            "string-width": "5.1.2",
            "string-width-cjs": "/string-width/4.2.3",
            "strip-ansi": "7.0.1",
            "strip-ansi-cjs": "/strip-ansi/6.0.1",
            "wrap-ansi": "8.1.0",
            "wrap-ansi-cjs": "/wrap-ansi/7.0.0",
        },
        transitive_closure = {
            "@isaacs/cliui": ["8.0.2"],
            "string-width": ["4.2.3", "5.1.2"],
            "strip-ansi": ["6.0.1", "7.0.1"],
            "wrap-ansi": ["7.0.0", "8.1.0"],
            "ansi-styles": ["6.2.1", "4.3.0"],
            "color-convert": ["2.0.1"],
            "color-name": ["1.1.4"],
            "ansi-regex": ["6.0.1", "5.0.1"],
            "emoji-regex": ["9.2.2", "8.0.0"],
            "is-fullwidth-code-point": ["3.0.0"],
            "eastasianwidth": ["0.2.0"],
        },
    )

and the deps, lc_deps and ref_deps look like this,

    deps = {
            ":.aspect_rules_js/{}/@[email protected]+@[email protected]/pkg".format(link_root_name): "@isaacs/cliui",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "is-fullwidth-code-point",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-convert",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-name",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "eastasianwidth",
        }
    lc_deps = {
            ":.aspect_rules_js/{}/@[email protected]+@[email protected]/pkg_pre_lc_lite".format(link_root_name): "@isaacs/cliui",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "string-width-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "strip-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "wrap-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-styles",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "ansi-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "emoji-regex",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "is-fullwidth-code-point",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-convert",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "color-name",
            ":.aspect_rules_js/{}/[email protected][email protected]/pkg".format(link_root_name): "eastasianwidth",
        }
    ref_deps = {
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "string-width",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "string-width-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "strip-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "strip-ansi-cjs",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "wrap-ansi",
            ":.aspect_rules_js/{}/[email protected][email protected]/ref".format(link_root_name): "wrap-ansi-cjs",
        }

@gregmagolan
Copy link
Member

@devversion This is a potential work-around for the material repository: https://github.com/angular/material.angular.io/pull/1208/files

@devversion
Copy link
Contributor Author

@gregmagolan Thanks for the investigation. So based on that, to me it sounds like the registry prefixes are currently just throwing off the logic, but in practice- with both variants (with registry prefix, or not), the aliases should be distinguishable, right? e.g.

with prefixes

  registry.npmjs.org/@isaacs/[email protected]:
    resolution: {integrity: sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==, registry: https://registry.yarnpkg.com/, tarball: https://registry.npmjs.org/@isaacs/cliui/-/cliui-8.0.2.tgz}
    name: '@isaacs/cliui'
    version: 8.0.2
    engines: {node: '>=12'}
    dependencies:
      string-width: registry.npmjs.org/[email protected]      <----- Same package name. No alias.
      string-width-cjs: registry.npmjs.org/[email protected]  <----- Different package name. Can be detected.
      strip-ansi: registry.npmjs.org/[email protected]
      strip-ansi-cjs: registry.npmjs.org/[email protected]
      wrap-ansi: registry.npmjs.org/[email protected]
      wrap-ansi-cjs: registry.npmjs.org/[email protected]
    dev: true

without prefix

  /@isaacs/[email protected]:
    resolution: {integrity: sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==}
    engines: {node: '>=12'}
    dependencies:
      string-width: 5.1.2                                             <----- No alias.
      string-width-cjs: /[email protected]                           <----- Different package name. Can be detected.
      strip-ansi: 7.0.1
      strip-ansi-cjs: /[email protected]
      wrap-ansi: 8.1.0
      wrap-ansi-cjs: /[email protected]
    dev: true

@gregmagolan
Copy link
Member

gregmagolan commented May 17, 2023

Essentially, yes. The logic breaks for the aliases somewhere along the way with the registry.npmjs.org/ prefixed versions. I started down the path of fixing it but it was more work than I had time for and with a viable work-around I'm going to put fixing the registry.npmjs.org/ prefix case on the backlog since we have other higher priority work at the moment.

@gregmagolan gregmagolan moved this from 🏗 In progress to 📋 Backlog in Open Source May 17, 2023
@devversion
Copy link
Contributor Author

FYI that team members often run into issues, even with the workaround, because pnpm import seems to behave different when running under yarn bazel. The registry prefixes are always added then- reverting the workarounds basically

@alexeagle alexeagle added the funding needed Contribute to https://opencollective.com/aspect-build label Aug 14, 2023
@jbedard
Copy link
Member

jbedard commented Oct 30, 2024

I think this may have been fixed with various lockfile cleanup/fixes. @devversion any idea if you're still running into this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working funding needed Contribute to https://opencollective.com/aspect-build
Projects
No open projects
Status: 📋 Backlog
Development

No branches or pull requests

4 participants