-
Notifications
You must be signed in to change notification settings - Fork 281
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
Add executable as a direct dependency #1082
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! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This fixes a bug introduced by bazelbuild#958. Before bazelbuild#958, `rootpath` on a Scala binary resolved to the wrapper binary so other rules could treat it as an opaque executable. After that PR `rootpath` now resolves to the JAR instead of the wrapper binary. Adding the executable back as a direct dependency fixes that.
d10ef07
to
d9062f9
Compare
# TODO: | ||
# Per Bazel documentation, we should avoid using collect_data. The core phases need to be updated | ||
# before we can make the adjustment. | ||
runfiles = ctx.runfiles(transitive_files = depset(transitive = runfiles), collect_data = True), | ||
runfiles = ctx.runfiles(transitive_files = depset(direct = direct, transitive = runfiles), collect_data = True), |
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.
Following https://groups.google.com/g/bazel-discuss/c/oEyEIr7vaF4/m/0E5QaK46AQAJ it also needs to be in runfiles.
Thanks!
If it's something else (because you mention something about other rules) then this again relates to my first sentence. I want us to merge this with a test. |
|
I added a testcase. It is slightly convoluted since |
Hi @cocreature, could you run lint (ci is failing on that)? |
e747a5b
to
349e712
Compare
Done, sorry I missed the failure. |
@ittaiz I think this is ready to be merged. |
This PR fixes a bug where
rootpath
on ascala_binary
resolved to the JAR instead of the wrapper binary.This fixes a bug introduced by
#958. Before #958,
rootpath
on a Scala binary resolved to the wrapper binary so otherrules could treat it as an opaque executable. After that PR
rootpath
now resolves to the JAR instead of the wrapper binary.
Adding the executable back as a direct dependency fixes that.
Happy to try and add a test for this but I need some guidance how and where to test this.
Description
Motivation