aspect-build / rules_ts

Bazel rules for the `tsc` compiler from http://typescriptlang.org
https://docs.aspect.build/rules/aspect_rules_ts
Apache License 2.0
102 stars 56 forks source link

[Bug]: copy_data_files_to_bin (for gather_runfiles) depends on whether transpiler is set #411

Open gzm0 opened 1 year ago

gzm0 commented 1 year ago

What happened?

It seems that ts_project doesn't expose this config option yet and the defaults are different.

Without transpiler

  1. data is passed to the ts_project rule here: https://github.com/aspect-build/rules_ts/blob/42094e84545d3420081da240819a123aa02b69ce/ts/defs.bzl#L406-L412

  2. The ts_project rule invokes gather_runfiles https://github.com/aspect-build/rules_ts/blob/42094e84545d3420081da240819a123aa02b69ce/ts/private/ts_project.bzl#L270-L275

  3. The default for copy_data_files_to_bin is False

With transpiler

  1. data is passed to js_library here: https://github.com/aspect-build/rules_ts/blob/42094e84545d3420081da240819a123aa02b69ce/ts/defs.bzl#L392-L400

  2. The default for js_library is True

What I'd expect

Version

Development (host) and target OS/architectures:

Output of bazel --version:

bazelisk --version
bazel 6.3.2

Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file:

rules_js: 1.31.0
rules_ts: 1.4.5

Language(s) and/or frameworks involved: None.

How to reproduce

Minimal steps, happy to provide more if necessary.

1. Add a filegroup with files in a different package to `data` of a `ts_project` (without `transpiler` set).
2. Build passes.
3. Set `transpiler` to `swc`.
4. Build fails (example failure below)
5. Switch `filegroup` to `js_library`
6. Build passes

Expected to find source file <the file> in '//package-of-ts-project', but instead it is in '//package-of-file'.

All source and data files that are not in the Bazel output tree must be in the same package as the
target so that they can be copied to the output tree in an action.

See https://docs.aspect.build/rules/aspect_rules_js/docs/#javascript for more context on why this is required.


### Any other information?

Happy to open a PR to align this, but I'd like to agree on the following first:
- Exact fix (add flag, align behavior, ???)
- Target branch (1.x, 2.x)?
alexeagle commented 12 months ago

The behavior with and without transpiler to be the same

I don't think we make this promise anywhere, though I can understand why it's unfortunate to make non-local changes when switching the transplier

There be an equivalent option to js_library on ts_project (the macro).

Is this just saying you need a boolean attribute to be exposed in the public API to control copy_data_files_to_bin?

It would be really great if you could find somewhere in our examples/e2e folder where you could reproduce the problem caused by your data files getting copied or not copied, and how the runfiles is involved, so we could study a bit more.

gzm0 commented 11 months ago

I don't think we make this promise anywhere, though I can understand why it's unfortunate to make non-local changes when switching the transplier

Fair enough. Although this one was particularly confusing to me because I'd expect the data attribute not to interact with the compilation/transpilation step at all (but only with downstream execution steps).

Is this just saying you need a boolean attribute to be exposed in the public API to control copy_data_files_to_bin?

Yes, pretty much (and maybe, but IIUC for 2.0.0 it's already too late, make it the same default, independent of whether transpiler is set).

It would be really great if you could find somewhere in our examples/e2e folder where you could reproduce the problem caused by your data files getting copied or not copied, and how the runfiles is involved, so we could study a bit more.

TBH, I do not understand the specific effect this has on the runfiles tree (I shamefully admit that when it comes to runfiles, I just fiddle with relative paths until my code can find the files).

The way I hit this issue is by depending on a filegroup:

ts_project(
  name = "test",
  // stuff
  data = [
    "//other-package:files" // <-- filegroup target containing source files
  ],
)

copy_to_bin will fail on //other-package:files, because it can only act on files in the invoking package (so for all I know the resulting runfiles tree would look the same).

To me, it feels this is reason enough to add the copy_data_files_to_bin options to ts_project but if you feel this needs some deeper understanding of how this actually affects runfiles (beyond what is documented in rules_js), I'm happy to do some digging.