Restructure scala provider, add JavaProvider #227
Conversation
|
I am still potentially fiddling with this a little. In particular, unsure if the tests properly capture what is going on yet. I had been delaying sending this out until some Bazel changes were released (as until then the 'feature' of interop with JavaProvider is slightly incomplete). However, the re-architecture here makes thinking about asks like "neverlink" much easier so just sending it out.... |
|
Thanks! |
|
If next week, makes sense to wait. Would avert the backwards incompatibility issue since my bazel PRs slightly amended the argument types to experimental java_common.create_provider. The changes are minimal (an adjustment to collect_jars and the code that constructs the provider in the _lib rule). I would probably continue using both the scala provider and java provider for this PR. Then, we can consider using just the java provider at a later date. I never explicitly tested java_test (somehow never occurred to me); however, it looks like it piggybacks on java_binary, which was thoroughly tested. |
|
Got you. I'll ping on the Bazel ticket and update here. |
|
so it looks like this fails on head on travis. Is that expected? |
scala/scala.bzl
Outdated
| compile_jars = next_cjars, | ||
| transitive_runtime_jars = transitive_rjars, | ||
| ) | ||
| # TODO(twigg): Linearization is concerning here. |
There was a problem hiding this comment.
is this something that has a fix on some open PR to bazel? Can we add a link to that if so?
There was a problem hiding this comment.
Sorry, yes, this was already fixed and is the reason this CL doesn't work on head.
scala/scala.bzl
Outdated
| @@ -786,6 +803,8 @@ def scala_repositories(): | |||
|
|
|||
| native.bind(name = "io_bazel_rules_scala/dependency/scalatest/scalatest", actual = "@scalatest//jar") | |||
|
|
|||
| # With providers changes, this is completely deprecatable once bazel release includes | |||
| # fixes to java rules using JavaProvider | |||
There was a problem hiding this comment.
can we change the implementation of this to just do the naive thing? We could make a java_library target that has the given exports and runtime_deps?
There was a problem hiding this comment.
Sadly, not yet, exports was what was specifically broken and fixed by:
However, might delay submitting this PR until that commit is released, in which case will definitely do that.
There was a problem hiding this comment.
It's pretty hard to see what is going on with bazel development without also figuring out the gerrit thing. I get that you all use it internally, but I never have and honestly it feels like some monstrous UI from 1993 or something. I am dreaming of the day all PRs go to github.
|
Stephen,
|
|
Sounds good to me. I already have a setup on my local machine for testing against Bazel@HEAD (from when making Bazel changes).
|
|
Sounds good. Waiting for your ping when you want us to review against Bazel HEAD and thanks for the clarifications! |
|
@sdtwigg I really want to try this PR with Bazel's HEAD. |
|
Take a look. Although, don't merge this CL as-is since I kept as two commits for now in case I need to jump back to the 0.5.1 version. |
The key change is that the scala provider has been completely restructured to match the structure of the JavaProvider. It no longer tracks exports separately but instead has the following fields: * outputs = contains all the outputs generated by the current rule (nothing or ijar and class_jar); however, no rules here actually use it. * compile_jars = contains all the jars dependent rules should compile with. Thus, ijar and exports.compile_jars * transitive_runtime_jars = contains all the jars needed to handle this target at runtime. Thus, class_jar plus (deps + exports + runtime_deps).transitive_runtime_jars The created java provider (used by dependent native java rules) uses just compile_jars and transitive_runtime_jars. In general, this change was seamless. The major exception is if you were specifically relying on only exports being re-exported by specifically gen_scrooge_srcjar. Beforehand, any jars produced by direct dependencies were not being exported to dependents but now they are. Note that, if you were using scrooge_scala_library instead, nothing should change. Other changes: * Use depset instead of set. (set docs already say set should be considered deprecated and just points to depset anyway) * Tests amended to exploit that JavaProvider is properly constructed. Note that things are still a little strange until Bazel releases incorporate some fixes to JavaProvider and native provider rules. Generally, only deps for java_library and java_binary work at the moment.
scala/scala.bzl
Outdated
| java_provider = target[java_common.provider] | ||
| compile_jars += java_provider.compile_jars | ||
| runtime_jars += java_provider.transitive_runtime_jars | ||
| elif hasattr(target, "scala"): |
There was a problem hiding this comment.
When will we hit this branch now? Won't the scala code hit the first provider?
There was a problem hiding this comment.
This code should not reach this branch.
I actually wanted to delete it, although was afraid of someone having weird custom rules appended that relied on the old provider being picked up. Then, they will fail-over to the raw file insertion. However, in retrospect, this was a silly thought and will remove.
Note, as future work, am tempted to create the custom no-ijar java_import variant so this raw file insertion can be phased out.
| outputs = None, | ||
| compile_jars = deps_jars.compile_jars, | ||
| transitive_runtime_jars = deps_jars.transitive_runtime_jars, | ||
| ) |
There was a problem hiding this comment.
Can we not use a java provider here?
There was a problem hiding this comment.
Will fix, good catch.
Remove scala_exports_to_java Prefer using JavaProvider over scala provider (can probably avoid using scala provider completely?)
|
Pushed some updates from feedback. As a small note, if using test_run.sh, make sure you clean when switching bazel versions/rules_scala versions. Some combination of changing both at once causes the identical test to fail for the deploy jars. I couldn't really dig into it further since everything works within a single version.... |
|
@sdtwigg do you want to change the travis file as part of your PR to minimal 0.5.2 rc2? |
|
@sdtwigg If I'm running locally with 0.5.2 rc4 do you see any reason not to cherry-pick your changes into my fork? anything unstable. From the chatter on the ticket 0.5.2 is coming out today. |
|
It should all work. I looked it over earlier and it is basically where I
want it excluding small, likely non-functional cleanups.
…On Jun 27, 2017 5:53 AM, "Ittai Zeidman" ***@***.***> wrote:
@sdtwigg <https://github.com/sdtwigg> If I'm running locally with 0.5.2
rc4 do you see any reason not to cherry-pick your changes into my fork?
anything unstable. From the chatter on the ticket
<bazelbuild/bazel#3086> 0.5.2 is coming out
today.
Main reason I need this change is because it will enable me to have
java_test depend on scala_library which in turn will allow me to use
$location expansion in jvm_flags AND get the runtime location (instead of
the source location). This is currently a blocker for me to use remote
execution. (This <bazelbuild/bazel#2475>
currently works for some native cases but not for skylark)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#227 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA9_-GfvF2TEv1pr44LSNerz0MULsRnNks5sIPskgaJpZM4N32Bn>
.
|
|
Great! 0.5.2 was finally released, can I add a commit to this PR to update the travis.yml from 0.4.5 to 0.5.2? We'll have good visibility then and can decide if we want to merge as is or wait for the cleanup?
|
|
What do you mean better support for strict deps and unused deps? Very
interesting!
…On Tue, Jun 27, 2017 at 18:06 Ittai Zeidman ***@***.***> wrote:
Great! 0.5.2 was finally released, can I add a commit to this PR to update
the travis.yml from 0.4.5 to 0.5.2? We'll have good visibility then and can
decide if we want to merge as is or wait for the cleanup?
Small headsup- we (Wix) want to make two significant changes in this area:
1. Refactor JUnit test
2. Better support "strict deps" and unused_deps.
The work on 2 started (based on these commits). If you'll refactor
soon then we'll rebase and resolve the conflicts but we it will be a
problem to do this in a few days time since the drift will be big.
*we'll open separate issues about these
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#227 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEJdlBQDq50wejwSq_aIp_ttWaRKJNpks5sIdFWgaJpZM4N32Bn>
.
|
|
:) I'll open a ticket in an hour or two. Just wanted to give the heads up.
On Wed, 28 Jun 2017 at 7:11 P. Oscar Boykin <notifications@github.com>
wrote:
… What do you mean better support for strict deps and unused deps? Very
interesting!
On Tue, Jun 27, 2017 at 18:06 Ittai Zeidman ***@***.***>
wrote:
> Great! 0.5.2 was finally released, can I add a commit to this PR to
update
> the travis.yml from 0.4.5 to 0.5.2? We'll have good visibility then and
can
> decide if we want to merge as is or wait for the cleanup?
> Small headsup- we (Wix) want to make two significant changes in this
area:
>
> 1. Refactor JUnit test
> 2. Better support "strict deps" and unused_deps.
> The work on 2 started (based on these commits). If you'll refactor
> soon then we'll rebase and resolve the conflicts but we it will be a
> problem to do this in a few days time since the drift will be big.
> *we'll open separate issues about these
>
> —
> You are receiving this because you commented.
>
>
> Reply to this email directly, view it on GitHub
> <
#227 (comment)
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AAEJdlBQDq50wejwSq_aIp_ttWaRKJNpks5sIdFWgaJpZM4N32Bn
>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#227 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIFxP50OlA-Cb4ittwYSBUUm4Hhl3Bks5sIdJcgaJpZM4N32Bn>
.
|
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
Yay! The build is green. @sdtwigg waiting for your OK (or small cleanup) so we can merge. |
|
Test this please |
|
Confirm. |
|
@sdtwigg thank you so much for your contributions! This has been a long outstanding debt :) |
* Restructure scala provider, add JavaProvider The key change is that the scala provider has been completely restructured to match the structure of the JavaProvider. It no longer tracks exports separately but instead has the following fields: * outputs = contains all the outputs generated by the current rule (nothing or ijar and class_jar); however, no rules here actually use it. * compile_jars = contains all the jars dependent rules should compile with. Thus, ijar and exports.compile_jars * transitive_runtime_jars = contains all the jars needed to handle this target at runtime. Thus, class_jar plus (deps + exports + runtime_deps).transitive_runtime_jars The created java provider (used by dependent native java rules) uses just compile_jars and transitive_runtime_jars. In general, this change was seamless. The major exception is if you were specifically relying on only exports being re-exported by specifically gen_scrooge_srcjar. Beforehand, any jars produced by direct dependencies were not being exported to dependents but now they are. Note that, if you were using scrooge_scala_library instead, nothing should change. Other changes: * Use depset instead of set. (set docs already say set should be considered deprecated and just points to depset anyway) * Tests amended to exploit that JavaProvider is properly constructed. Note that things are still a little strange until Bazel releases incorporate some fixes to JavaProvider and native provider rules. Generally, only deps for java_library and java_binary work at the moment. * Using JavaProvider, requires Bazel 0.5.2 Remove scala_exports_to_java Prefer using JavaProvider over scala provider (can probably avoid using scala provider completely?) * update minimum bazel version to 0.5.2
The key change is that the scala provider has been completely
restructured to match the structure of the JavaProvider. It no longer
tracks exports separately but instead has the following fields:
outputs = contains all the outputs generated by the current rule
(nothing or ijar and class_jar); however, no rules here actually use it.
compile_jars = contains all the jars dependent rules should compile
with. Thus, ijar and exports.compile_jars
transitive_runtime_jars = contains all the jars needed to handle this
target at runtime. Thus, class_jar plus (deps + exports +
runtime_deps).transitive_runtime_jars
The created java provider (used by dependent native java rules) uses
just compile_jars and transitive_runtime_jars.
In general, this change was seamless. The major exception is if you were
specifically relying on only exports being re-exported by specifically
gen_scrooge_srcjar. Beforehand, any jars produced by direct dependencies
were not being exported to dependents but now they are. Note that, if
you were using scrooge_scala_library instead, nothing should change.
Other changes:
Use depset instead of set. (set docs already say set should be
considered deprecated and just points to depset anyway)
Tests amended to exploit that JavaProvider is properly constructed.
Note that things are still a little strange until Bazel releases
incorporate some fixes to JavaProvider and native provider rules.
Generally, only deps for java_library and java_binary work at the
moment.