Switch to JarsToLabels provider and rework/cleanup scala_import #477
Conversation
|
Currently failing on the strict deps tests ( |
| @@ -1,114 +1,49 @@ | |||
| #intellij part is tested manually, tread lightly when changing there | |||
| #if you change make sure to manually re-import an intellij project and see imports | |||
| #are resolved (not red) and clickable | |||
There was a problem hiding this comment.
why did we delete the comment here? Seems like it is still relevant.
There was a problem hiding this comment.
Overlooked it while porting this code over. I'll add it back (and actually prove out that this works for IntelliJ).
There was a problem hiding this comment.
Comment added back in the body of the method.
|
Now we're failing a bit further: My |
|
What’s the motivation here? |
|
I was aiming to clean up the code and use newer APIs. The export change has been redacted in the more recent commit and was there simply because I hadn't tried all the tests yet. The changes still aren't passing yet, I'll take a look later today. |
|
Two additional points:
|
andyscott
changed the title
|
How would we use the aspect when we need to get the list in the compilation action? I'm very rusty w.r.t aspects but I sort of remembered aspects are for meta-execution |
scala/scala_import.bzl
Outdated
| def _scala_import_impl(ctx): | ||
| target_data = _code_jars_and_intellij_metadata_from(ctx.attr.jars) |
There was a problem hiding this comment.
@andyscott why did you remove all the private methods? current code feels a bit like a wall of text
|
When defining a rule you can specify that an aspect be automatically evaluated for a particular attribute. A jars-to-labels aspect could evaluate over all deps, runtime_deps, etc, for scala rules and automatically provide the I don't think it's work exploring that in this PR but it might be a nice way to organize the logic down the road. |
|
I ran into #476 and noticed this PR to refactor scala_import. I opened a PR to this branch to add a I'm not very familiar with the DefaultInfo provider, and the doc is a bit sparse. Hopefully this is the right approach. |
|
I think there is a bug here with the exports. |
|
I've tried to write the failing test but couldn't 😠 From the code it sounds like your current version (where |
|
This definitely needs a bit more work and the testing is tricky. I can take a look over the second half of next week. |
Add files to DefaultInfo provider in scala_import
|
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 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 here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@jjudd looks like @googlebot is really on top of its game and you'll need to sign the CLA. |
|
It’s quite the opposite actually.
Once a PR has several contributors it breaks down.
@jjudd needs to consent via text and even then it’s up to us maintainers
manually
…On Fri, 13 Apr 2018 at 18:31 andy g scott ☜ ***@***.***> wrote:
@jjudd <https://github.com/jjudd> looks like @googlebot
<https://github.com/googlebot> is really on top of its game and you'll
need to sign the CLA.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#477 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF4T_vubBI8-5qtN1XAkECT2_gH2rks5toMSygaJpZM4TNV84>
.
|
|
I consent to my contributions going in |
| for file in jar.files: | ||
| all_jar_files.append(file) | ||
| if not file.basename.endswith("-sources.jar"): | ||
| direct_binary_jars += [file] |
There was a problem hiding this comment.
Why use += here but append above? Can we use append here too?
There was a problem hiding this comment.
It's proof that I am not as consistent as I wish to be. (Good catch, I'll fix it!)
There was a problem hiding this comment.
I'm pretty sure that I wrote the .append and andy wrote the +=. It seems it is proof that I don't always read the surrounding code as well as I should :D
There was a problem hiding this comment.
if we're touching this, wdyt about moving it to a private method as well? same level of abstraction for the top level
| for file in jar.files: | ||
| all_jar_files.append(file) | ||
| if not file.basename.endswith("-sources.jar"): | ||
| direct_binary_jars += [file] |
There was a problem hiding this comment.
if we're touching this, wdyt about moving it to a private method as well? same level of abstraction for the top level
| def _scala_import_java_info(ctx, direct_binary_jars): | ||
| # merge all deps, exports, and runtime deps into single JavaInfo instances | ||
|
|
||
| s_deps = java_common.merge(_collect(JavaInfo, ctx.attr.deps)) |
There was a problem hiding this comment.
what's the s_ notation and do we need it?
| for entry in ctx.attr.exports: | ||
| if JavaInfo in entry: | ||
| for jar in entry[JavaInfo].compile_jars: | ||
| lookup[jar.path] = entry.label |
There was a problem hiding this comment.
if we won't change this to ctx.label (per exports discussion) then can we refactor these two loops to a common method? if ctx.label doesn't change they're exactly the same
|
I'm diving in to this PR + IntelliJ today. I've been running off of a branch that uses it and I'm experiencing some red, non-clickable symbols and trying to figure out if this is the cause. |
|
can we make sure to merge master on this? I added a test that we don't use incompatible features and it is easy to break that test since we don't automatically retest against master. |
|
I did some digging into the IntelliJ plugin today. It doesn't look like it supports the new JavaInfo provider. It appears to be a hybrid of the legacy java and java_output providers. I'm also not certain how to create a JavaInfo provider that has the information we need, considering that I think that JavaInfo now has everything needed to support the IntelliJ plugin. I'm not 100% sure, but an initial reading of the code suggests everything is there. Based on the responses I get on my thread in bazel-discuss, I may take a crack at modifying the IntelliJ plugin to support the new JavaInfo as well as the legacy provider. I see two options for this PR:
Option 2 would unblock this PR from being merged as soon as it is finished, however it's cumbersome. Option 1 would unblock the PR as soon as the new IntelliJ plugin is released, assuming we are ok with making a backwards incompatible change. |
|
Do you plan to also move scala library to the new provider? That’s a bigger
change which I’m still hesitant about
…On Thu, 19 Apr 2018 at 7:04 James Judd ***@***.***> wrote:
I did some digging into the IntelliJ plugin today. It doesn't look like it
supports the new JavaInfo
<https://docs.bazel.build/versions/master/skylark/lib/JavaInfo.html>
provider. It appears to be a hybrid of the legacy java
<https://docs.bazel.build/versions/master/skylark/lib/JavaSkylarkApiProvider.html>
and java_output
<https://docs.bazel.build/versions/master/skylark/lib/java_output.html>
providers.
I'm also not certain how to create a JavaInfo provider that has the
information we need, considering that java_common.create_provider is
deprecated in favor of the JavaInfo provider. I asked more about that
here: https://groups.google.com/forum/#!topic/bazel-discuss/hQ7qOoZ53Fw
I think that JavaInfo now has everything needed to support the IntelliJ
plugin. I'm not 100% sure, but an initial reading of the code suggests
everything is there.
Based on the responses I get on my thread in bazel-discuss, I may take a
crack at modifying the IntelliJ plugin to support the new JavaInfo as well
as the legacy provider.
I see two options for this PR:
#1 <#1> We modify the
IntelliJ plugin to support JavaInfo
#2 <#2> We return a
legacy java provider everywhere we return a JavaInfo provider.
#2 <#2> would unblock
this PR from being merged as soon as it is finished, however it's
cumbersome. #1 <#1> would
unblock the PR as soon as the new IntelliJ plugin is released, assuming we
are ok with making a backwards incompatible change.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#477 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF1dUSlx11E0BsuZ60HEI1-nMUOfwks5tqAzYgaJpZM4TNV84>
.
|
|
I think we should move scala_library to the new provider. I don't think it should be part of this PR, unless there's a reason to move both at the same time. |
|
I had a concern about the new provider.
I think mainly that it encapsulates things differently and so label
collection (for strict deps) is complicated.
If you want to take a stab and we’ll review together that might work
…On Thu, 19 Apr 2018 at 7:57 James Judd ***@***.***> wrote:
I think we should move scala_library to the new provider. I don't think it
should be part of this PR, unless there's a reason to move both at the same
time.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#477 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF-JAx2ubjv4PPGsetAPnJh0y2fGhks5tqBkogaJpZM4TNV84>
.
|
|
Perhaps we can refactor so that provider creation is handled by a private method that can initially create the legacy structures to support IntelliJ? In parallel, we can work to update the IntelliJ plugin to support |
|
@andyscott I like that idea. I'll take a stab at that today. |
|
@ittaiz let's start here, see how this PR goes, and based on that and the label stamping work let's decide what we want to do moving forward. |
|
I added IntelliJ support, rebased master, and fixed the incompatible errors. The PR to @andyscott's branch is here https://github.com/andyscott/rules_scala/pull/2. The incompatible errors are fixed in this commit: andyscott@3dd70d7 The IntelliJ support is in this commit: andyscott@a457c06 |
|
The IntelliJ support this time around includes support for source jars. The IntelliJ plugin needs a modification to automatically identify them. Until then you need to click 'Attach downloaded source' or 'Attach source' in IntelliJ to get them to show up. |
|
can we send a PR to the intellj plugin to get it able to use JavaInfo? I would really like to move to modern providers everywhere and not keep the old style we have currently. |
|
I'm happy to work on that as well. I will have time next week. We're close to switching all of engineering over to Bazel and are pushing hard to get that done. |
|
just a note: caching is working like gangbusters for us in our CI.
we are experimenting with laptops writing to a separate cache as well. I
would look at setting up caching sooner than later to get good CI
performance.
…On Thu, Apr 19, 2018 at 3:50 PM, James Judd ***@***.***> wrote:
I'm happy to work on that as well. I will have time next week. We're close
to switching all of engineering over to Bazel and are pushing hard to get
that done.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#477 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEJdko5vpTrmY6QorHCjAHed7GfjOjUks5tqT7ogaJpZM4TNV84>
.
--
P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin
|
|
Awesome. That's fantastic to hear. It's the next thing on our list. |
|
ok, @andyscott @jjudd this PR feels a bit lost right now (thanks to all of us :) ) |
WIP.
This reworks
scala_importunder the assumption that you're using a newish version of Bazel. Jars-to-labels mappings are now provided with a provider.I haven't tested this yet with IntelliJ.