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

Expose java stub template through @bazel_tools #12994

Closed
wants to merge 1 commit into from

Conversation

illicitonion
Copy link
Contributor

Currently rules_kotlin downloads this via raw.githubusercontent.com:

https://github.com/bazelbuild/rules_kotlin/blob/96c70d1d3cc788b03406f7db6a659c78d311c3d8/kotlin/internal/repositories/release_repositories.bzl#L55-L62

https://github.com/bazelbuild/rules_kotlin/blob/96c70d1d3cc788b03406f7db6a659c78d311c3d8/kotlin/internal/jvm/jvm.bzl#L135-L138

This will allow rules_kotlin to point at a file from Bazel proper,
rather than downloading it via the internet.

@google-cla google-cla bot added the cla: yes label Feb 11, 2021
@jin jin added the team-Rules-Java Issues for Java rules label Mar 1, 2021
Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

I don't mind exposing the template, just the implementation of it.

Is it possible to move the java_stub_template.txt file into the tools/java/ directory and then depend on it from rules/java? This way there would be no need for a copy genrule.

@illicitonion
Copy link
Contributor Author

Absolutely - done! This is now exposed as @bazel_tools//tools:java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt without any copies - thanks for taking a look :)

@comius
Copy link
Contributor

comius commented Apr 8, 2021

Absolutely - done! This is now exposed as @bazel_tools//tools:java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt without any copies - thanks for taking a look :)

Do you need the whole path? I thought you could put it to tools/jdk package directly.

@illicitonion
Copy link
Contributor Author

Absolutely - done! This is now exposed as @bazel_tools//tools:java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt without any copies - thanks for taking a look :)

Do you need the whole path? I thought you could put it to tools/jdk package directly.

If I do that, when I reference it as a resources it ends up in the root of the jar, rather than in the package, and that makes the ResourceFileLoader sad. I could side-step ResourceFileLoader to not do the package-name-prefixing when trying to read the file, if you'd prefer?

@comius
Copy link
Contributor

comius commented Apr 9, 2021

Absolutely - done! This is now exposed as @bazel_tools//tools:java/com/google/devtools/build/lib/bazel/rules/java/java_stub_template.txt without any copies - thanks for taking a look :)

Do you need the whole path? I thought you could put it to tools/jdk package directly.

If I do that, when I reference it as a resources it ends up in the root of the jar, rather than in the package, and that makes the ResourceFileLoader sad. I could side-step ResourceFileLoader to not do the package-name-prefixing when trying to read the file, if you'd prefer?

No sidestepping ResourceFileLoader doesn't seem nice. Can filegroup rule help in this case (with path)? Or even with the previous version you had with the file in original location, i.e. replacing genrule with filegroup?

@illicitonion
Copy link
Contributor Author

Filegroup doesn't appear to have a path attribute? https://docs.bazel.build/versions/master/be/general.html#filegroup

@comius
Copy link
Contributor

comius commented Apr 9, 2021

Filegroup doesn't appear to have a path attribute? https://docs.bazel.build/versions/master/be/general.html#filegroup

Sorry, you're right.

@illicitonion
Copy link
Contributor Author

Would you rather I fix up CI for this version, or go back to a genrule which does cp? (Or something else :))

@comius
Copy link
Contributor

comius commented Apr 9, 2021

Would you rather I fix up CI for this version, or go back to a genrule which does cp? (Or something else :))

I'd like to have "pretty" code, but I don't have time to play around myself to figure out if there is something nicer.

I'll accept the genrule with 'cp'. This way you don't have to deal with peculiarities of the tests and other options like creating out of place directory structure or tinkering with ResourceFileLoader seem worse.

I don't know by heart if you could use a regular filegroup instead of cp genrule in the old version of the code. If you are willing to try; otherwise it's fine too.

@illicitonion
Copy link
Contributor Author

I played around with filegroups for a while to no effect - reverted to the original cp approach

@comius
Copy link
Contributor

comius commented Apr 9, 2021

I played around with filegroups for a while to no effect - reverted to the original cp approach

Thanks!

@bazel-io bazel-io closed this in 8ace6db Apr 16, 2021
illicitonion added a commit to illicitonion/bazel that referenced this pull request Aug 2, 2021
bazelbuild#12994 added the file, but it's
currently exposed as:
`@build_tools//tools:java/java_stub_template.txt`

This fixes it up to be properly exposed as:
`@build_tools//tools/java:java_stub_template.txt`

Which makes more sense :)
illicitonion added a commit to illicitonion/bazel that referenced this pull request Aug 2, 2021
bazelbuild#12994 added the file, but it's
currently exposed as:
`@build_tools//tools:java/java_stub_template.txt`

This fixes it up to be properly exposed as:
`@build_tools//tools/java:java_stub_template.txt`

Which makes more sense :)
illicitonion added a commit to illicitonion/bazel that referenced this pull request Aug 4, 2021
bazelbuild#12994 added the file, but it's
currently exposed as:
`@build_tools//tools:java/java_stub_template.txt`

This fixes it up to be properly exposed as:
`@build_tools//tools/java:java_stub_template.txt`

Which makes more sense :)
illicitonion added a commit to illicitonion/bazel that referenced this pull request Aug 4, 2021
bazelbuild#12994 added the file, but it's
currently exposed as:
`@build_tools//tools:java/java_stub_template.txt`

This fixes it up to be properly exposed as:
`@build_tools//tools/java:java_stub_template.txt`

Which makes more sense :)
illicitonion added a commit to illicitonion/bazel that referenced this pull request Aug 4, 2021
bazelbuild#12994 added the file, but it's
currently exposed as:
`@build_tools//tools:java/java_stub_template.txt`

This fixes it up to be properly exposed as:
`@build_tools//tools/java:java_stub_template.txt`

Which makes more sense :)
bazel-io pushed a commit that referenced this pull request Aug 4, 2021
#12994 added the file, but it's
currently exposed as:
`@build_tools//tools:java/java_stub_template.txt`

This fixes it up to be properly exposed as:
`@build_tools//tools/java:java_stub_template.txt`

Which makes more sense :)

Closes #13790.

PiperOrigin-RevId: 388677326
illicitonion added a commit to illicitonion/rules_kotlin that referenced this pull request Dec 5, 2023
This was added in bazelbuild/bazel#12994 (and
fixed in bazelbuild/bazel#13790).

This has been included in all Bazel releases since 5.0.0, which is the
minimum supported Bazel version of rules_kotlin according to https://github.com/bazelbuild/rules_kotlin/blob/master/README.md

This prevents having a build-time dependency on access to
raw.githubusercontent.com.
illicitonion added a commit to illicitonion/rules_kotlin that referenced this pull request Dec 5, 2023
This was added in bazelbuild/bazel#12994 (and
fixed in bazelbuild/bazel#13790).

This has been included in all Bazel releases since 5.0.0, which is the
minimum supported Bazel version of rules_kotlin according to https://github.com/bazelbuild/rules_kotlin/blob/master/README.md

This prevents having a build-time dependency on access to
raw.githubusercontent.com.
illicitonion added a commit to illicitonion/rules_kotlin that referenced this pull request Dec 5, 2023
This was added in bazelbuild/bazel#12994 (and
fixed in bazelbuild/bazel#13790).

This has been included in all Bazel releases since 5.0.0, which is the
minimum supported Bazel version of rules_kotlin according to https://github.com/bazelbuild/rules_kotlin/blob/master/README.md

This prevents having a build-time dependency on access to
raw.githubusercontent.com.
restingbull pushed a commit to bazelbuild/rules_kotlin that referenced this pull request Dec 8, 2023
This was added in bazelbuild/bazel#12994 (and
fixed in bazelbuild/bazel#13790).

This has been included in all Bazel releases since 5.0.0, which is the
minimum supported Bazel version of rules_kotlin according to https://github.com/bazelbuild/rules_kotlin/blob/master/README.md

This prevents having a build-time dependency on access to
raw.githubusercontent.com.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants