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.

Sign up for GitHub

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

Jump to bottom

//test:ScalaBinaryInGenrule fails with Bazel HEAD #280

Closed
katre opened this issue Sep 26, 2017 · 16 comments · Fixed by #325
Closed

//test:ScalaBinaryInGenrule fails with Bazel HEAD #280

katre opened this issue Sep 26, 2017 · 16 comments · Fixed by #325

Comments

@katre
Copy link
Member

katre commented Sep 26, 2017

As of bazelbuild/bazel@655a529, java_common.create_provider requires the ctx.actions object to be passed as the first argument. rules_scala needs to be updated (here at least:

rules_scala/scala/scala.bzl

Line 627 in 85e154c

java_provider = java_common.create_provider(
) to pass this.

Failure message:

$ bazel-dev build //test:ScalaBinaryInGenrule
...
ERROR: /usr/local/google/home/jcater/.cache/bazel/_bazel_jcater/3ab7d86a57881a7a5e9f250ec1d3fb5f/external/io_bazel_rules_scala/third_party/plugin/src/main/BUILD:5:1: in scala_library_for_plugin_bootstrapping rule @io_bazel_rules_scala//third_party/plugin/src/main:dependency_analyzer: 
Traceback (most recent call last):
	File "/usr/local/google/home/jcater/.cache/bazel/_bazel_jcater/3ab7d86a57881a7a5e9f250ec1d3fb5f/external/io_bazel_rules_scala/third_party/plugin/src/main/BUILD", line 5
		scala_library_for_plugin_bootstrapping(name = 'dependency_analyzer')
	File "/usr/local/google/home/jcater/.cache/bazel/_bazel_jcater/3ab7d86a57881a7a5e9f250ec1d3fb5f/external/io_bazel_rules_scala/scala/scala.bzl", line 661, in _scala_library_impl
		_lib(ctx, True)
	File "/usr/local/google/home/jcater/.cache/bazel/_bazel_jcater/3ab7d86a57881a7a5e9f250ec1d3fb5f/external/io_bazel_rules_scala/scala/scala.bzl", line 627, in _lib
		java_common.create_provider(compile_time_jars = scalaattr.co..., <3 more arguments>)
In java_common.create_provider the value of use_ijar is True. Make sure the first argument of the function is the ctx.actions object.
WARNING: errors encountered while analyzing target '//test:ScalaBinaryInGenrule': it will not be built
@ittaiz
Copy link
Member

ittaiz commented Sep 26, 2017 via email

@katre
Copy link
Member Author

katre commented Sep 26, 2017

Ok. Please note that this is already failing on our CI system: http://ci.bazel.io/blue/organizations/jenkins/Global%2Frules_scala/detail/rules_scala/197/pipeline

@ittaiz
Copy link
Member

ittaiz commented Sep 26, 2017 via email

@johnynek
Copy link
Member

If I'm reading this correctly, we can't support both 0.6.0 and 0.5.x with this change.

Is this right?

Is there any kind of macro of function we can use the bzl file to switch the call based on version? Not everyone is on the latest.

@katre
Copy link
Member Author

katre commented Sep 26, 2017

@iirina, do you have any suggestions on how to be forward-compatible with this?

@ittaiz
Copy link
Member

ittaiz commented Sep 26, 2017 via email

This was referenced Sep 26, 2017
Closed
Closed
@johnynek
Copy link
Member

maybe what we should do is add a bazel_0_7 tag put changes that only go into that there. When bazel 0.7.0 is released, we can make that master and make a bazel 0_6 branch for the stuff that only works with bazel 0.6 (and 0.5.3 also).

@ittaiz
Copy link
Member

ittaiz commented Sep 30, 2017

@johnynek I think this can reduce the velocity of adding new features and given our current scala.bzl god file it can probably greatly reduce it.
IMHO we should do something along what you suggested a while back and that is to define a 1.0 milestone (we should agree on the contents in a different issue) and once we get there we should probably upgrade the version every time we break the API or require a new bazel version.
In that scenario we can consider backporting features when there is contributor capacity or we think it's critical.
WDYT?

@johnynek
Copy link
Member

This sounds like a good plan to me.

But I might propose something else: version along with bazel. So rules_scala 0.8.0 is the first version that works with bazel 0.8.0, 0.8.1 the next. Since bazel seems to be making a plan to keep stable for any fixed 0.x for all y 0.x.y, this might make it easier for users.

As far as 1.0 features, I think we are basically there. I would like to be able to pick what version of scala to use and at least support 2.11 and 2.12 dynamically (by config rather than by branch). I would also like to close #256. But other than that we have:

  • full java interop
  • support for jmh benchmarks
  • thrift and proto
  • basic repl support
  • tut for typechecked docs.

This seems like actually a pretty good menu. I'm not even sure what we should add beyond that?

A rule to autogenerate build files using zinc would be really cool to allow people to easily factor targets that are taking too long, but I don't see that as a 1.0 showstopper.

@ittaiz
Copy link
Member

ittaiz commented Sep 30, 2017

I don't understand how the above plan accounts for breaking changes by us.
let's say we have a 0.8.0 tag which requires bazel 0.8.0 and this is currently what master points to.
Not we want (for some reason) to break our API. We should somehow indicate this in the version.
What we can do is maybe do a Major.minor_bazelVersion scheme.
For the above example it would mean we have 2.0_0.8.0 (2.0 is just as an example) and then that breaking change would mean we tag it as 3.0_0.8.0.
We can use 0.minor.minorMinor_bazelVersion if you think running too high in the "major" versions will scare people.

Re 1.0 features I definitely agree about 2.11/2.12 (I'd actually like scala_toolchain support but since I can't see us working on it I don't want to block on it).
I'd also add good strict-scala-deps support. This means its merged as the default. We should probably not block until we have the "smart" mode although I'd really also like that in.

@johnynek
Copy link
Member

I would argue we don't make breaking changes without pairing it with a new bazel version. This will make it somewhat simpler for users to deal with.

My concern is that compatibility matrices get really challenging for people to keep track of. Additionally, I think our users should generally not have to care about most of the internal workings of the rule. I think then interface we present should be very stable and shouldn't break builds.

What kind of changes do you envision wanting to make that would break builds?

@ittaiz
Copy link
Member

ittaiz commented Sep 30, 2017

I'm not sure but we did some breaking changes w.r.t jvm_flags and the sorts.
We sometimes see we've diverged (due to misunderstanding and not by intent) from java_* rules and we want to fix it.
I also want to extract the logic of test discovery from rules_scala to a separate repo since it has no relation to scala but rather junit and many other bazel users want this to ease their maven migration. This is currently blocked by bazelbuild/bazel#2475.

@hsyed
Copy link

hsyed commented Oct 19, 2017

I'm getting the same error mentioned above after upgrading to 0.7.0 -- I am using the latest version of the rules. Adding --nouse_ijars does not solve the problem. Is there a workaround for this ?

@ittaiz
Copy link
Member

ittaiz commented Oct 19, 2017

Oscar, I'm going to make the change and require 0.7 as the minimum

@johnynek
Copy link
Member

@ittaiz I thought that some Google’s made a PR that dynamically checked the existence of some methods and fixed it?

In any case, we can move to 0.7. We use java providers pretty heavily so we should be on the latest. Maybe it can help us fix things with intellij.

@ittaiz
Copy link
Member

ittaiz commented Oct 19, 2017 via email

@ittaiz ittaiz mentioned this issue Oct 29, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

create provider conditionally based on existence of attribute bazelbuild/rules_scala
4 participants
@johnynek @katre @hsyed @ittaiz