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

fix(esbuild): allow whitespace within args #2998

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

jbedard
Copy link
Collaborator

@jbedard jbedard commented Oct 4, 2021

idk about the write_file... wdyt? This also makes it public which we could avoid until being more certain about it.

index.bzl Outdated Show resolved Hide resolved
@alexeagle
Copy link
Collaborator

Error on CI looks legit

ERROR: /workdir/packages/esbuild/test/define/BUILD.bazel:16:8: Bundling Javascript packages/esbuild/test/define/main.ts [esbuild] failed: (Exit 1): _bundle_esbuild_launcher.sh failed: error executing command
--


error:
 Could not read from file: 
/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1201/execroot/build_bazel_rules_nodejs/$(execpath
 :shim.js)

@jbedard
Copy link
Collaborator Author

jbedard commented Oct 12, 2021

Looks like the skylib write_file doesn't do ctx.expand_make_variables so that's not going to work... the two easiest things I can think of:

  • replace the args_file with args_json and do expand_variables on that json before writing to the file
  • essentially what I originally had... our own write_file that does expand_variables on the content, this could live alongside our expand_variables util

@alexeagle
Copy link
Collaborator

I see, param_file was never the right thing to use.

does it work if you expand variables in the string passed to write_file? I think that's your first suggestion in the last comment. If it works that sounds short.

I'd rather not maintain yet another copy of copy_file

@jbedard
Copy link
Collaborator Author

jbedard commented Oct 12, 2021

See the latest version.

I updated the macro to pass the json.encode(args) as a string and used ctx.expand_location() to write the .json file within the rule itself. Instead of defining another rule essentially just for that ctx.expand_location.

I think passing a string of JSON (of an unknown size) to a rule is fine?

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

seems reasonable to me - @mattem do you want another pass before I merge?

@alexeagle alexeagle merged commit 3446425 into bazel-contrib:stable Oct 28, 2021
alexeagle pushed a commit that referenced this pull request Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants