ci: Windows support for tsec_test. #43924
Conversation
|
@devversion hi Paul, could you take a look at this? |
Thanks @uraj. This looks great to me. I tested it locally and it works as expected.
Obviously this resulted in a lot of additional code in the tsec starlark file, but I think that is just a necessary trade-off for supporting windows. Do you have any plans porting this file over to the tsec repository so that other repositories could use this as well? like angular/components?
tools/tsec.bzl
Outdated
| if not use_runfiles: | ||
| paths[name].append(bazel_out_root + "/" + path) | ||
|
|
||
| nm_root = tslib_info.node_modules_root |
There was a problem hiding this comment.
Nit: I personally would vote for not abbreviating it. Seems short enough but much clearer.
| nm_root = tslib_info.node_modules_root | |
| node_module_root = tslib_info.node_modules_root |
There was a problem hiding this comment.
Done
tools/tsec.bzl
Outdated
| paths["*"] = [nm_root + "/*", nm_root + "/" + "@types/*"] | ||
| compiler_options["typeRoots"] = [src_base_dir + "/" + p[:-2] for p in paths["*"]] |
There was a problem hiding this comment.
Small suggestion on how this could be done without the substring indices:
| paths["*"] = [nm_root + "/*", nm_root + "/" + "@types/*"] | |
| compiler_options["typeRoots"] = [src_base_dir + "/" + p[:-2] for p in paths["*"]] | |
| type_roots = [node_module_root, node_module_root + "/@types"] | |
| paths["*"] = ["%s/*" % r for r in type_roots] | |
| compiler_options["typeRoots"] = ["%s/%s/*" % (src_base_dir, r) for r in type_roots] |
There was a problem hiding this comment.
Done
tools/tsec.bzl
Outdated
| def _generate_tsconfig(target, base_tsconfig): | ||
| tsconfig = {} | ||
| base_url = "/".join([".."] * len(target.label.package.split("/"))) | ||
| def _generate_tsconfig(bazel_out_root, target, base_tsconfig, use_runfiles): |
There was a problem hiding this comment.
We should use the common terminology for this path.
| def _generate_tsconfig(bazel_out_root, target, base_tsconfig, use_runfiles): | |
| def _generate_tsconfig(bin_dir_path, target, base_tsconfig, use_runfiles): |
There was a problem hiding this comment.
Done.
tools/tsec.bzl
Outdated
| def _generate_tsconfig(bazel_out_root, target, base_tsconfig, use_runfiles): | ||
| tsconfig = {"bazel": True} | ||
| pkg_base_dir = "/".join([".."] * len(target.label.package.split("/"))) | ||
| src_base_dir = pkg_base_dir if use_runfiles else "/".join([".."] * len(bazel_out_root.split("/")) + [pkg_base_dir]) |
There was a problem hiding this comment.
Can you please add a small comment explaining that this is usually a path resolving to the workspace root relatively to the current package, but in the case of no runfiles, this is a relative path resolving to the Bazel package folder in source tree (by going up to the execroot first, and then back to the package directory)?
There was a problem hiding this comment.
Done.
tools/tsec.bzl
Outdated
| output = ctx.outputs.out, | ||
| content = generated_tsconfig_content, | ||
| generated_tsconfig_content = _generate_tsconfig( | ||
| out.root.path, |
There was a problem hiding this comment.
This should just use ctx.bin_dir.path
|
@devversion Thanks for the review. I'm consulting with Google's open source programs office about moving this file into tsec repo, since this is technically re-publishing third-party code (tsec has a special release setup since it doesn't expect any 3p contribution). Once that's done, I will move this to tsec's package and start working on angular/components integration. |
LGTM
LGTM
Reviewed-for: fw-security
note: there is no security stuff in here, just infra work, so using my global approval rights to push this through.
Reviewed-for: global-approvers
Hello @uraj, @devversion,
This PR only merges cleanly to master, not to 13.0.x. I'm going to hold off on merging to master at the moment to ensure the two branches don't diverse unnecessarily.
Please either retarget this PR, and if needed open a second PR for 13.0.x specifically. Thanks!
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the |
|
merge-assistance: can you please help @uraj land this as there seems to be something off about master and 13.0.x diverging prior to this PR. thank you! |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the |
|
@IgorMinar I've opened another PR (#44010) for |
atscott
added
target: minor
target: patch
and removed
action: cleanup
target: patch
target: minor
labels
|
The CI failure doesn't seem to relate to the code change. Should we trigger another run? |
|
@uraj I attempted to rebase but pushed before the rebase completed so that resulted in the PR being closed. Are you able to push back to this branch? Otherwise, we'll have to start a new PR. Also, my understanding is that there is a conflict with the |
Contents of generated tsconfig for tsec_test now depend on whether Bazel uses symlinked runfiles for nodejs_test. The current implementation assumes that symlinked runfiles are not available on Windows.
|
This PR was merged into the repository by commit 7ac2a8c. |
|
Just a heads up: As mentioned before, this had conflicts with the patch ( |
Contents of generated tsconfig for tsec_test now depend on whether Bazel uses symlinked runfiles for nodejs_test. The current implementation assumes that symlinked runfiles are not available on Windows.
Implementation-wise, this PR detects the platform on which Bazel is running and infers whether runfiles would be available to
nodejs_test. And based on that, thetsec_testmacro generates differenttsconfig.jsonfiles to adapt to different directory layouts.@bazel/runfilesis not used since there are too many path-searching dynamics that we can't control inside the TypeScript APIs.A new tsec version (0.1.11) is required to actually activate Windows support.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
The text was updated successfully, but these errors were encountered: