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

2.0 RELEASE #1671

Closed
16 of 21 tasks
gregmagolan opened this issue Apr 20, 2024 · 7 comments
Closed
16 of 21 tasks

2.0 RELEASE #1671

gregmagolan opened this issue Apr 20, 2024 · 7 comments
Assignees
@gregmagolan
Labels
tracking issue This is a tracking issue
Milestone
Breaking Changes ...

Comments

@gregmagolan
Copy link
Member

gregmagolan commented Apr 20, 2024 •
edited by jbedard
Loading

rules_js 2.x branch: https://github.com/aspect-build/rules_js/tree/2.x

FEATURES

RCs ARE OUT

These are ready to try,

https://github.com/aspect-build/rules_js/releases/tag/v2.0.0-rc2

https://github.com/aspect-build/rules_cypress/releases/tag/v0.6.0-rc0
https://github.com/aspect-build/rules_esbuild/releases/tag/v0.21.0-rc1
https://github.com/aspect-build/rules_jasmine/releases/tag/v2.0.0-rc0
https://github.com/aspect-build/rules_jest/releases/tag/v0.22.0-rc0
https://github.com/aspect-build/rules_rollup/releases/tag/v2.0.0-rc0
https://github.com/aspect-build/rules_swc/releases/tag/v2.0.0-rc0
https://github.com/aspect-build/rules_ts/releases/tag/v3.0.0-rc0
https://github.com/aspect-build/rules_webpack/releases/tag/v0.16.0-rc0

BLOCKING TODOS (before 2.0 final)

PUNTED TO POST 2.0 RELEASE (non-breaking follow-ups)

DOWNSTREAM RULE SET CHANGES

BREAKING CHANGES

User facing changes

npm_package

npm_translate_lock

npm_import

js_filegroup => js_info_files

declarations => types

WORKSPACE

Dependencies

Drop Bazel 5 support

Other

Internal changes that are BREAKING for downstream rule sets but not for end users

js_binary

NpmPackageInfo

NpmPackageStoreInfo

JsInfo

Other

Places to update when release is out
@gregmagolan gregmagolan added this to the Breaking Changes (2.0) milestone Apr 20, 2024
@github-actions github-actions bot added the untriaged Requires traige label Apr 20, 2024
@gregmagolan gregmagolan added tracking issue This is a tracking issue and removed untriaged Requires traige labels Apr 20, 2024
@gregmagolan gregmagolan self-assigned this Apr 20, 2024
@gregmagolan
Copy link
Member Author

cc @jbedard

This was referenced May 7, 2024
Closed
Closed
@jbedard jbedard mentioned this issue May 14, 2024
Merged
@bazaglia
Copy link
Contributor

I know the 2.0 release is still in alpha, but 1st party npm dependencies are failing to me on RBE. The following target builds sucessfully on RBE on rules_js 1.x:

npm_link_all_packages(name = "node_modules")

ts_project(
    name = "compile_ts",
    srcs = glob(
        [
            "src/**/*.ts",
        ],
        exclude = [
            "src/**/*.spec.ts",
        ],
    ),
    declaration = True,
    root_dir = "src",
    source_map = True,
    transpiler = partial.make(
        swc,
        root_dir = "src",
        source_maps = "true",
        swcrc = "//backend:.swcrc",
    ),
    deps = [":node_modules"],
)

npm_package(
    name = "pkg",
    srcs = [":compile_ts"],
    include_runfiles = False,
    visibility = ["//visibility:public"],
)

The change of the default value for include_runfiles didn't affect me since I was already providing False before. The only change I made was in the target name (to pkg).

The target builds sucessfully without RBE. If I set include_runfiles = True it works with RBE too. But I suspect this is wrong, since I've been using False since rules_js 1.x where it works.

The error I get is:

Copying files to directory backend/packages/logging/pkg failed: (Exit 1): copy_to_directory failed: error executing CopyToDirectory command (from target //backend/packages/logging:pkg) 
  (cd /home/andre/.cache/bazel/_bazel_andre/f5cfe5f2674b3facfa2b14436fac5b0a/execroot/_main && \
  exec env - \
  external/aspect_bazel_lib~~toolchains~copy_to_directory_linux_arm64/copy_to_directory bazel-out/aarch64-fastbuild/bin/backend/packages/logging/pkg_config.json '')
# Configuration: a1829c87b28b44c4110093c03e1e47fc8ba5855812d3fc30aa05e0cb682bc1bf
# Execution platform: @@local_config_platform//:host
2024/05/15 00:24:05 lstat bazel-out/aarch64-fastbuild/bin/backend/packages/logging/node_modules/@opentelemetry/api failed: lstat bazel-out/aarch64-fastbuild/bin/backend/node_modules/.aspect_rules_js/@opentelemetry+api@1.8.0/node_modules/@opentelemetry/api: no such file or directory

The @opentelemetry/api is a dependency declared on the package.json.

@gregmagolan
Copy link
Member Author

gregmagolan commented May 15, 2024 •
edited
Loading

The change of the default value for include_runfiles didn't affect me since I was already providing False before. The only change I made was in the target name (to pkg).

The target builds sucessfully without RBE. If I set include_runfiles = True it works with RBE too. But I suspect this is wrong, since I've been using False since rules_js 1.x where it works.

Interesting. Thanks for the report. I'm not sure off hand what could be different tho its may be related to npm package sources no longer being in the JsInfo types where previously they were. The change was actually on the 1.x branch: #1554. Can you check if you also see the same issue on the latest 1.x release?

It might be that that change combined with some other change on the 2.x branch leads to the issue you're seeing. Either way, the fact that it is trying to copy a file from an npm package that is not there is probably a bug in the ruleset. The lstat is coming from the copy_to_directory golang tool. Looks like from the Realpath function in there: https://github.com/aspect-build/bazel-lib/blob/474e680caddadb8f95074406a42526b46c3e2cf1/tools/common/file.go#L35.

@gregmagolan
Copy link
Member Author

@bazaglia Can you compare the generated {name}_config.json file from the 1.x working case and the 2.x broken case? The copy_to_directory/npm_package rule generates that file as a manifest of what to copy.

https://github.com/aspect-build/bazel-lib/blob/474e680caddadb8f95074406a42526b46c3e2cf1/lib/private/copy_to_directory.bzl#L481

I'd be interested to see the difference if you're able to send me those files over Bazel Slack.

@bazaglia
Copy link
Contributor

@gregmagolan You are right, there are some differences in the generated pkg_config.json file. I compared with rules_js 1.41.1 so #1554 is already included in there. It didn't seem to be the cause of the issue. I sent you the files on Slack.

@gregmagolan gregmagolan mentioned this issue May 28, 2024
Merged
@gregmagolan
Copy link
Member Author

gregmagolan commented May 28, 2024 •
edited
Loading

@gregmagolan You are right, there are some differences in the generated pkg_config.json file. I compared with rules_js 1.41.1 so #1554 is already included in there. It didn't seem to be the cause of the issue. I sent you the files on Slack.

I was able to repro and I have a fix for a case which looks like what you're hitting: #1762

This was referenced May 28, 2024
Merged
Merged
Strum355 pushed a commit to sourcegraph/sourcegraph-public-snapshot that referenced this issue Jun 4, 2024
@gregmagolan
chore(bazel): upgrade to rules_js 2.0 RC (#63022)
2d3d918 
Bumps to rules_js (and friends) to 2.0 RCs.

This brings in performance improvements for analysis phase since npm package depsets and now much smaller. It also adds support for pnpm v9 and allows for linking js_library targets as 1p deps instead of npm_package targets. See aspect-build/rules_js#1671 for more details.

## Test plan

CI

## Changelog
alexeagle added a commit that referenced this issue Aug 13, 2024
@alexeagle
chore: docs updates for 2.0
9111c5e 
- Add a migration guide, adapted from #1671
- Add eslint in a few spots so it's more likely users discover it
- Promote Aspect Workflows for CI/CD
@alexeagle alexeagle mentioned this issue Aug 13, 2024
Merged
alexeagle added a commit that referenced this issue Aug 13, 2024
@alexeagle
chore: docs updates for 2.0 (#1887)
c996609 
* chore: docs updates for 2.0

- Add a migration guide, adapted from #1671
- Add eslint in a few spots so it's more likely users discover it
- Promote Aspect Workflows for CI/CD

* Update migrate_2.md
@alexeagle
Copy link
Member

Marking this closed as the 2.0.0 release is now published

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gregmagolan gregmagolan

Labels
tracking issue This is a tracking issue
Projects
None yet
Milestone
Breaking Changes (2.0)
Development

No branches or pull requests
3 participants
@alexeagle @gregmagolan @bazaglia