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

[ts_project] Windows outDir workaround causes error when used with generated tsconfig, due to target name generation #2884

Closed
matthewjh opened this issue Aug 24, 2021 · 4 comments
Labels
Can Close? We will close this in 30 days if there is no further activity

Comments

@matthewjh
Copy link
Contributor

matthewjh commented Aug 24, 2021

🐞 bug report

Affected Rule

ts_project

Is this a regression?

n/a

Description

When running on Windows, in order to use project references between ts_project rules without issues, the docs recommend the following workaround:

  1. When using Project References, TypeScript will expect to verify that the outputs of referenced projects are up-to-date with respect to their inputs. (This is true even without using the --build option). When using a non-sandboxed spawn strategy, tsc can read the sources from other ts_project rules in your project, and will expect that the tsconfig.json file for those references will indicate where the outputs were written. However the outDir is determined by this Bazel rule so it cannot be known from reading the tsconfig.json file. This problem is manifested as a TypeScript diagnostic like error TS6305: Output file '/path/to/execroot/a.d.ts' has not been built from source file '/path/to/execroot/a.ts'. As a workaround, you can give the Windows “fastbuild” output directory as the outDir in your tsconfig file. On other platforms, the value isn’t read so it does no harm. See https://github.com/bazelbuild/rules_nodejs/tree/stable/packages/typescript/test/ts_project as an example. We hope this will be fixed in a future release of TypeScript; follow No way to use composite project where outDir is determined by build tool microsoft/TypeScript#37378

This works when the outDir config is manually written into the tsconfig.json file referred to by the ts_project rule, but not when the tsconfig.json file is generated by the ts_project rule using the rule attributes.

E.g.

{
    "extends": "./tsconfig-extended.json",
    "compilerOptions": {
        "sourceMap": true,
        "outDir": "../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a",
    }
}

works

but

ts_project(
    composite = True,
    extends =  "//packages/typescript/test/ts_project:tsconfig",
    tsconfig = {
       "compilerOptions": {
            "outDir": "../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a",
        }
    },
    # Intentionally not syncing this option from tsconfig, to test validator suppression
    # source_map = True,
    validate = False,
    visibility = ["//packages/typescript/test:__subpackages__"],
    # Use @babel/parser since the package.json is required to resolve "typings" field
    # Repro of #2044
    deps = [
        "@npm//@babel/parser",
        "@npm//@babel/types",
    ],
)

errors with

ERROR: C:/users/matt/documents/github/rules_nodejs_fork/rules_nodejs/packages/typescript/test/ts_project/a/BUILD.bazel:12:11: //packages/typescript/test/ts_project/a:tsconfig: illegal output file name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.js' in rule //packages
/typescript/test/ts_project/a:tsconfig: invalid target name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.js': target names may not contain up-level references '..'
ERROR: C:/users/matt/documents/github/rules_nodejs_fork/rules_nodejs/packages/typescript/test/ts_project/a/BUILD.bazel:12:11: //packages/typescript/test/ts_project/a:tsconfig: illegal output file name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.d.ts' in rule //packag
es/typescript/test/ts_project/a:tsconfig: invalid target name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.d.ts': target names may not contain up-level references '..'
ERROR: error loading package 'packages/typescript/test/ts_project/a': Package 'packages/typescript/test/ts_project/a' contains errors
INFO: Elapsed time: 0.387s

This is important for us as we need project references to work properly, but don't want to check in tsconfig files with boilerplate.

This error seems to be caused by ts_project declaring outputs by joining the outDir attribute with the src file names:

  if len(ctx.outputs.js_outs):
        pkg_len = len(ctx.label.package) + 1 if len(ctx.label.package) else 0
        json_outs = [
            ctx.actions.declare_file(_join(ctx.attr.out_dir, src.short_path[pkg_len:]))
            for src in ctx.files.srcs
            if src.basename.endswith(".json") and src.is_source

For now I am working around this by patching ts_project.bzl to comment out the following:

# These options are always passed on the tsc command line so don't include them
        # in the tsconfig. At best they're redundant, but at worst we'll have a conflict
        if "outDir" in compiler_options.keys():
            out_dir = compiler_options.pop("outDir")

This causes the tsconfig.compilerOptions.outDir attribute not to influence the above code, while still allowing it to flow through to the generated tsconfig file.

I don't know what a proper fix would look like.

🔬 Minimal Reproduction

https://github.com/matthewjh/rules_nodejs/tree/outdir_generated_tsconfig_repro

bazel build //packages/typescript/test/ts_project/a:tsconfig

🔥 Exception or Error


```
ERROR: C:/users/matt/documents/github/rules_nodejs_fork/rules_nodejs/packages/typescript/test/ts_project/a/BUILD.bazel:12:11: //packages/typescript/test/ts_project/a:tsconfig: illegal output file name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.js' in rule //packages
/typescript/test/ts_project/a:tsconfig: invalid target name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.js': target names may not contain up-level references '..'
ERROR: C:/users/matt/documents/github/rules_nodejs_fork/rules_nodejs/packages/typescript/test/ts_project/a/BUILD.bazel:12:11: //packages/typescript/test/ts_project/a:tsconfig: illegal output file name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.d.ts' in rule //packag
es/typescript/test/ts_project/a:tsconfig: invalid target name '../../../../../bazel-out/x64_windows-fastbuild/bin/packages/typescript/test/ts_project/a/a.d.ts': target names may not contain up-level references '..'
ERROR: error loading package 'packages/typescript/test/ts_project/a': Package 'packages/typescript/test/ts_project/a' contains errors
INFO: Elapsed time: 0.387s
```

🌍 Your Environment

Operating System:

  
  Windows 10
  

Output of bazel version:

  
 Bazelisk version: v1.5.0
Build label: 4.1.0
Build target: bazel-out/x64_windows-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri May 21 11:17:01 2021 (1621595821)
Build timestamp: 1621595821
Build timestamp as int: 1621595821

  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
 stable
  

Anything else relevant?

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Nov 23, 2021
@alexeagle alexeagle removed the Can Close? We will close this in 30 days if there is no further activity label Nov 23, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label May 25, 2022
@github-actions
Copy link

This issue was automatically closed because it went 30 days without any activity since it was labeled "Can Close?"

@alexeagle
Copy link
Collaborator

I think rules_js is the best answer for this, since there's no need for outDir anymore (actions run with working directory in the output tree)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? We will close this in 30 days if there is no further activity
Projects
None yet
Development

No branches or pull requests

2 participants