-
Notifications
You must be signed in to change notification settings - Fork 248
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
Resolve tools to include runfiles #437
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
@oquenchil is there anything you'd like me to change to get this merged? |
df637fd
to
7c8a534
Compare
563c1c3
to
cfb8ad7
Compare
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.
A couple of questions/comments
tools/build_defs/native_tools/BUILD
Outdated
@@ -18,7 +18,7 @@ make_tool( | |||
|
|||
native_tool_toolchain( | |||
name = "built_make", | |||
path = "make/bin/make", | |||
path = "$(execpath :make_tool)/bin/make", |
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.
seems a bit odd. I'd expect $(execpath :make_tool)
to be a path to the tool and target = ":make_tool"
should be sufficient. Do you think it'd be difficult to update this to not require both target
and path
?
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.
$(execpath :make_tool)
is the output from a rules_foreign_cc
build which doesn't generate an executable target directly ( you have to do the dance of creating a filegroup
and an executable target via a wrapper or another cc_*
rule. This issue is along the lines of the same problem that #428 is looking to address too; there may be multiple tools required in the PATH
but I guess in the emscripten case that this could be done by a shell wrapper that sets up the path so that binaryen
is present in the context that emscripten
is expecting it to be.
Also note that I'd made this observation in the tool_access.bzl
file that it'd be easier if this was an executable target than a freeform path as we need to special case between absolute and relative paths.
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.
One last question but I think it otherwise looks good.
642fbe9
to
0195136
Compare
Is #490 related to this in any way? |
0195136
to
b278a03
Compare
b278a03
to
07fbc03
Compare
Good job, this is it... It will put an end to this repo |
7942b5d
to
f8170d7
Compare
Can you explain why you feel that way? |
f8170d7
to
f0cbd45
Compare
This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_foreign_cc! |
This PR was automatically closed because it went 30 days without a reply since it was labeled "Can Close?" |
Fixes #433