-
Notifications
You must be signed in to change notification settings - Fork 524
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
feat(esbuild): add support for toolchains #2704
feat(esbuild): add support for toolchains #2704
Conversation
caac006
to
e61e0c8
Compare
e61e0c8
to
b7c2be6
Compare
b7c2be6
to
c6c2e56
Compare
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 ℹ️ Googlers: Go here for more info. |
|
Oh, well, we seem to be at an impasse @googlebot |
Yeah... that showed you 🤖 |
Re the update script, any objections to adding esbuild-darwin-arm64 and esbuild-linux-arm64 as well? I can add them in a follow-up PR if that's easier. |
Makes sense to me, I'll add them in here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty simple change, the user WORKSPACE is so much nicer now :)
Allowing users to specify their own esbuild version and adding plugin support don't mix well, so I think I'm going to remove that part, which simplifies this feature quite a bit. |
f88ea58
to
56f1f60
Compare
binary_path = "bin/esbuild", | ||
exec_compatible_with = [ | ||
"@platforms//os:linux", | ||
"@platforms//cpu:x86_64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be aarch64. I had a user report that arm64 did not work as an alias in these constraints - not sure if that also applies to the darwin_arm64 build as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the wrong one for linux here (copy paste). Does arm64
work? I don't have a machine to test this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me either :-) But the user reported arm64 did not work: ankitects/esbuild_toolchain#2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting, I see, thanks
8317b5f
to
389feaf
Compare
389feaf
to
099a667
Compare
Add toolchain support for esbuild, removing the need for the tool attribute. Users can load the esbuild_repositories macro and call it within their WORKSPACE, this will by default define esbuild binaries for Windows, MacOS and Linux (all amd64). esbuild package now uses toolchains, and the tools attribute has been removed. By default, toolchains for Windows, Mac (amd64, arm64) and Linux (amd64, arm64) are included. See the docs for configure_esbuild_toolchain for defining custom toolchains Add the following to the `WORKSPACE` file (changing `@npm` as required), see the docs for an alternate load method ``` load("@npm//@bazel/esbuild:esbuild_repositories.bzl", "esbuild_repositories") esbuild_repositories() ```
BREAKING CHANGE
Add toolchain support for esbuild, removing the need for the
tool
attribute. Users can load theesbuild_repositories
macro and call it within theirWORKSPACE
, this will by default define esbuild binaries for Windows, MacOS and Linux (all amd64).TODO:
Thanks to @dae for the discussion in #2592 and the prototype repo
Relnotes:
esbuild
package now uses toolchains, and thetools
attribute has been removed. By default, toolchains for Windows, Mac (amd64
,arm64
) and Linux (amd64
,arm64
) are included.See the docs for
configure_esbuild_toolchain
for defining custom toolchainsAdd the following to the
WORKSPACE
file (changing@npm
as required), see the docs for an alternate load method