Skip to content

Conversation

@jbedard
Copy link
Member

@jbedard jbedard commented Dec 10, 2025

Ref aspect-build/rules_js#2574

Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: yes
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

Support bazel path mapping across compilation modes etc.

Test plan

  • Covered by existing test cases

@jbedard jbedard requested a review from dzbarsky December 10, 2025 20:59
@aspect-workflows
Copy link

aspect-workflows bot commented Dec 10, 2025

Test (Bazel 6.x) (Test)

All tests were cache hits

179 tests (100.0%) were fully cached saving 9s.


Test (Bazel 7.x) (Test)

All tests were cache hits

179 tests (100.0%) were fully cached saving 13s.


Test (Bazel 8.x) (Test)

All tests were cache hits

179 tests (100.0%) were fully cached saving 12s.


Bazel-7

e2e/bzlmodules

All tests were cache hits

1 test (100.0%) was fully cached saving 32ms.


Bazel-8

e2e/bzlmodules

All tests were cache hits

1 test (100.0%) was fully cached saving 30ms.


Bazel-6

e2e/external_dep

All tests were cache hits

6 tests (100.0%) were fully cached saving 262ms.


Bazel-7

e2e/external_dep

All tests were cache hits

6 tests (100.0%) were fully cached saving 289ms.


Bazel-8

e2e/external_dep

All tests were cache hits

6 tests (100.0%) were fully cached saving 291ms.


Bazel-7

e2e/smoke

All tests were cache hits

2 tests (100.0%) were fully cached saving 107ms.


Bazel-8

e2e/smoke

All tests were cache hits

2 tests (100.0%) were fully cached saving 149ms.


Bazel-6

e2e/test

All tests were cache hits

179 tests (100.0%) were fully cached saving 9s.


Bazel-7

e2e/test

All tests were cache hits

179 tests (100.0%) were fully cached saving 13s.


Bazel-8

e2e/test

All tests were cache hits

179 tests (100.0%) were fully cached saving 12s.


Bazel-6

e2e/worker

All tests were cache hits

6 tests (100.0%) were fully cached saving 209ms.


Bazel-7

e2e/worker

All tests were cache hits

6 tests (100.0%) were fully cached saving 287ms.


Bazel-8

e2e/worker

All tests were cache hits

6 tests (100.0%) were fully cached saving 363ms.


Buildifier      Format

Copy link
Collaborator

@dzbarsky dzbarsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you check that to_output_relative_path works as we expect?

) + (
[ctx.executable.protoc_gen_connect_query] if ctx.attr.gen_connect_query else []
),
env = {"BAZEL_BINDIR": ctx.bin_dir.path},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm can you check with -s what gets sent here? I'm not convinced this .path won't break it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be an issue basically anywhere a js_binary is run?

SUBCOMMAND: # //ts/test:use_tsconfig_file [action 'TsValidateOptions ts/test/use_tsconfig_file_params.validation', configuration: acb2e42730a6aa54083f4bef3b595e5f72ad2d5bca2cad0bf6ae4c508646ead4, execution platform: @@platforms//host:host, mnemonic: TsValidateOptions]
(cd /private/var/tmp/_bazel_jason/e367b9c6c43239d8dcd71785defc8675/execroot/_main && \
  exec env - \
    BAZEL_BINDIR=bazel-out/darwin_arm64-fastbuild/bin \
    PATH=/bin:/usr/bin:/usr/local/bin \
  bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/_main~ext~npm_typescript/validator_/validator ts/test/src_tsconfig2.json ts/test/use_tsconfig_file_params.validation @@//ts/test:use_tsconfig_file ts/test '{"allow_js":false,"composite":false,"declaration":true,"declaration_map":false,"emit_declaration_only":false,"incremental":false,"isolated_typecheck":false,"no_emit":false,"out_dir":"outdir2","preserve_jsx":false,"resolve_json_module":false,"root_dir":"","source_map":false,"ts_build_info_file":""}')
# Configuration: acb2e42730a6aa54083f4bef3b595e5f72ad2d5bca2cad0bf6ae4c508646ead4
# Execution platform: @@platforms//host:host

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean this PR has essentially no effect?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i believe so

@jbedard jbedard requested a review from alexeagle December 10, 2025 23:53
@jbedard
Copy link
Member Author

jbedard commented Dec 11, 2025

did you check that to_output_relative_path works as we expect?

It only uses short_path so I think we're good?

@jbedard jbedard marked this pull request as ready for review December 15, 2025 19:01
@jbedard jbedard marked this pull request as draft December 15, 2025 21:22
@jbedard
Copy link
Member Author

jbedard commented Dec 15, 2025

Blocked on aspect-build/rules_js#2581

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

Successfully merging this pull request may close these issues.

2 participants