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

Cannot depend on ts_project that only contains declaration files #72

Closed
duarten opened this issue Jul 5, 2022 · 5 comments · Fixed by #86
Closed

Cannot depend on ts_project that only contains declaration files #72

duarten opened this issue Jul 5, 2022 · 5 comments · Fixed by #86
Assignees

Comments

@duarten
Copy link

duarten commented Jul 5, 2022

Consider the following modification to examples/ts_project_dep:

// a/a.d.ts
export interface Foo { }
# a/BUILD.bazel
load("@aspect_rules_ts//ts:defs.bzl", "ts_project")

ts_project(
    name = "ts",
    srcs = ["a.d.ts"],
    declaration = True,
    visibility = ["//visibility:public"],
)
// b/b.ts
import { Foo } from '../a/a'
# a/BUILD.bazel
load("@aspect_rules_ts//ts:defs.bzl", "ts_project")

ts_project(
    name = "ts",
    srcs = ["b.ts"],
    deps = ["//a:ts"],
)

Running bzl build //examples/ts_project_dep/... results in the following error:

ERROR: /Users/duarte.nunes/code/rules_ts/examples/ts_project_dep/b/BUILD.bazel:3:11: in ts_project_rule rule //examples/ts_project_dep/b:ts:
Traceback (most recent call last):
	File "/Users/duarte.nunes/code/rules_ts/ts/private/ts_project.bzl", line 158, column 47, in _ts_project_impl
		inputs = copy_files_to_bin_actions(ctx, inputs),
	File "/private/var/tmp/_bazel_duarte.nunes/f58cdaf222cf7d2c1c0920674e48c308/external/aspect_bazel_lib/lib/private/copy_to_bin.bzl", line 109, column 36, in copy_files_to_bin_actions
		return [copy_file_to_bin_action(ctx, file, is_windows = is_windows) for file in files]
	File "/private/var/tmp/_bazel_duarte.nunes/f58cdaf222cf7d2c1c0920674e48c308/external/aspect_bazel_lib/lib/private/copy_to_bin.bzl", line 42, column 13, in copy_file_to_bin_action
		fail(_file_in_different_package_error_msg(file, ctx.label))
Error in fail:
Expected to find file a.d.ts in //examples/ts_project_dep/b, but instead it is in //examples/ts_project_dep/a.

To use copy_to_bin, either move a.d.ts to //examples/ts_project_dep/b, or move the copy_to_bin
target to //examples/ts_project_dep/a using:

    buildozer 'new copy_to_bin a.d' //examples/ts_project_dep/a:__pkg__
    buildozer 'add srcs a.d.ts' //examples/ts_project_dep/a:a.d
    buildozer 'new_load @aspect_bazel_lib//lib:copy_to_bin.bzl copy_to_bin' //examples/ts_project_dep/a:__pkg__
    buildozer 'add visibility //examples/ts_project_dep/b:__subpackages__' //examples/ts_project_dep/a:a.d
@alexeagle
Copy link
Member

The issue is that there are no actions to perform when ts_project is given only .d.ts inputs

$ bazel aquery //examples/ts_project_dep/a:ts
INFO: Analyzed target //examples/ts_project_dep/a:ts (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
action 'Copying file examples/ts_project_dep/a/tsconfig.json'
  Mnemonic: CopyFile
  Target: //examples/ts_project_dep/a:ts
  Configuration: k8-fastbuild
  Execution platform: @local_config_platform//:host
  ActionKey: df9f69878ca80b87cda3ef6eb5471a9430a16c7dfd18326fe4330f4ec27afd79
  Inputs: [examples/ts_project_dep/a/tsconfig.json]
  Outputs: [bazel-out/k8-fastbuild/bin/examples/ts_project_dep/a/tsconfig.json]
  ExecutionInfo: {local: 1, no-cache: 1, no-remote: 1, no-remote-cache: 1, no-remote-exec: 1, no-sandbox: 1}
  Command Line: (exec /bin/bash \
    -c \
    'cp -f "$1" "$2"' \
    '' \
    examples/ts_project_dep/a/tsconfig.json \
    bazel-out/k8-fastbuild/bin/examples/ts_project_dep/a/tsconfig.json)
# Configuration: 914a02700104c3896ec11fa05d39cefe62f60c91acb9be9448be6c8af25ce1cc
# Execution platform: @local_config_platform//:host
  ExecutionInfo: {local: 1, no-cache: 1, no-remote: 1, no-remote-cache: 1, no-remote-exec: 1, no-sandbox: 1}

so then when b depends on it, we try to copy that file to the bin folder.

Changing that a/BUILD.bazel to use js_library instead fixes it:

load("@aspect_rules_js//js:defs.bzl", "js_library")

js_library(
    name = "ts",
    srcs = ["a.d.ts"],
    visibility = ["//visibility:public"],
)

so the question here is, what should be the behavior? Either ts_project should error in your case because there are no actions to perform, or it should just act the same as a js_library.

@duarten
Copy link
Author

duarten commented Jul 11, 2022

That makes sense. I was using this to debug an unrelated problem, so I didn't even stop to think.

so the question here is, what should be the behavior? Either ts_project should error in your case because there are no actions to perform, or it should just act the same as a js_library.

I see two reasons to have ts_project behave like js_library. One is that maybe because of its name, the intuition is to use ts_project when there are typescript sources involved, even though its purpose is just to translate from .ts files into corresponding .js and .d.ts files. The other is that you can have a package that contained or will contain .ts files along with .d.ts files, and you don't want to then have to modify your build rules.

@alexeagle
Copy link
Member

Hmm, my intuition was to go the other way like in that linked PR, because it confuses the mental model for users if there are two different rules that behave the same.
Also if we call this ts_project then you might expect it to do some type-checking of the supplied .d.ts file rather than blindly copy it across to the output folder. I think we could have a feature request here for that behavior - and then we'd want to reserve ts_project for that semantics.

So I think we should print the buildozer instructions to change to js_library for this case, at least for now.

@thesayyn
Copy link
Member

I agree with Alex that .d.ts files should not be copied directly. The question is whether ts_project should type check .d.ts files or not. If yes then this issue gets resolved as a result of that change.

@alexeagle
Copy link
Member

filed #88

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 a pull request may close this issue.

3 participants