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

Make more use of the toolchain #399

Closed
wants to merge 4 commits into from
Closed

Conversation

johnynek
Copy link
Member

@johnynek johnynek commented Jan 20, 2018

This is a small change that I think sets us up to make a scala 2.12 toolchain and merge scala 2.12 into master.

I think the next steps are:

  • make toolchain for scalatest
  • make toolchain for specs
  • make toolchain scrooge
  • make toolchain for scalapb
  • merge the above back into 2.12 branch, and make a 2.12 toolchain for each
  • copy the 2.12 toolchains into master selecting 2.11 as the default

@johnynek
Copy link
Member Author

cc @ittaiz @gkk-stripe

@johnynek
Copy link
Member Author

relates to #14

@johnynek
Copy link
Member Author

strange... the OSX has a class not found on the bazel ci, but linux is clear. I wonder if there is some difference in how platforms handle toolchains? Not sure why. Also I tested locally on osx. Will investigate.

cc @katre

@johnynek
Copy link
Member Author

well evidently things aren't so reproducible. After running bazel clean things now fail for me with this error:

st-oscar1:rules_scala oscar$ bazel build test/...
INFO: Analysed 441 targets (0 packages loaded).
INFO: Found 441 targets...
ERROR: /Users/oscar/oss/rules_scala/src/java/io/bazel/rulesscala/test_discovery/BUILD:3:1: scala //src/java/io/bazel/rulesscala/test_discovery:test_discovery failed: Worker process did not return a WorkResponse:

---8<---8<--- Start of log, file at /private/var/tmp/_bazel_oscar/cff0ec2cf472ebc5cb4402685dc7edf3/bazel-workers/worker-36-Scalac.log ---8<---8<---
/private/var/tmp/_bazel_oscar/cff0ec2cf472ebc5cb4402685dc7edf3/execroot/io_bazel_rules_scala/bazel-out/host/bin/external/io_bazel_rules_scala/src/java/io/bazel/rulesscala/scalac/scalac: Cannot locate runfiles directory. (Set $JAVA_RUNFILES to inhibit searching.)
---8<---8<--- End of log ---8<---8<---
INFO: Elapsed time: 0.560s, Critical Path: 0.04s
FAILED: Build did NOT complete successfully

@johnynek
Copy link
Member Author

okay, @katre I removed the worker which was an ctx.executable pointing at the worker rule and it seems to work (on bazel ci, let's see if travis agrees which runs more tests which are not bazel targets, but driven with a shell script, e.g. reproducibility tests).

Why did that work on linux, but not OSX? Should it have worked?

@johnynek
Copy link
Member Author

now, it seems that specs targets fail with strict deps, but not scalatest. It is not clear to me why.

Any ideas @ittaiz ?

@johnynek
Copy link
Member Author

Okay, it seems that specs also had a path of bringing in scala-library and scala-compiler. My guess is that somehow, this winds up looking like two separate jars to the strict deps system with the change somehow. Then we get an error.

This is pretty hand wavy... but it seems to work if we just remove the dependencies from the specs target and fix an aspect test not to expect them.

@ittaiz
Copy link
Member

ittaiz commented Jan 21, 2018

Well done!
I'll take a look at the specs2 issue but I think it might be related to an edge case we have with strict deps where scala lib and scala reflect are identified as transitive dependencies for some reason.
I was thinking of solving it this weekend but didn't have the time :)
Why do we need the other toolchains?
Can't we just make sure we use versions that has both 2.11 and 2.12 artifacts and then incorporate the "scala_mvn" function (or whatever it's called) into the toolchain?

Copy link
Member

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

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

ok.
Re the specs2 test I think it's ok. I think we resolved the underlying reason a while ago and didn't notice.
I think we should roughly define what we'd like the user experience to be and derive back from there.

Ideally I'd like someone to be able to run with either 2.11 or 2.12 toolchains (both supplied by us) which will bring the relevant jars (of std-lib/specs/scalatest/scrooge/proto).
WDYT?

@@ -45,8 +45,6 @@ def _rule_impl(ctx):
"@io_bazel_rules_scala//specs2:specs2",
"@scala//:scala-xml",
"@scala//:scala-parser-combinators",
"@scala//:scala-library",
Copy link
Member

Choose a reason for hiding this comment

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

@chaoren does this mandate changes in the plugin? So that it will take these dependencies from the toolchain?

@@ -9,6 +9,9 @@ toolchain_type(
scala_toolchain(
name = 'default_toolchain_impl',
scalacopts = [],
library = "//external:io_bazel_rules_scala/dependency/scala/scala_library",
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this is the way to go? If I understand it correctly this still requires me to change the scala.bzl#scala_repositories method to download 2.12.

@@ -9,6 +9,9 @@ toolchain_type(
scala_toolchain(
name = 'default_toolchain_impl',
scalacopts = [],
library = "//external:io_bazel_rules_scala/dependency/scala/scala_library",
compiler = "//external:io_bazel_rules_scala/dependency/scala/scala_compiler",
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using //external: dependencies instead of @io_bazel_rules_scala//dependency/scala/scala_compiler?

Copy link
Member Author

Choose a reason for hiding this comment

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

To make a more minimal change. Before toolchains, we only had bind as a tool that downstream repos could set these values and there are many interchangeable versions that work.

@johnynek
Copy link
Member Author

@ittaiz I agree that we want a 2.11 and 2.12 toolchain (heck, maybe even 2.10, I guess). But we are not super close. If we put all the jars in the repo in one toolchain, we kind of forces a lot of jar dependencies on everyone (scala, scalapb, scrooge, specs, etc..). I would prefer to set up each group separately.

Do you think we should merge this as a small step, or do we wait until we have it possible to switch out 2.11 for 2.12?

@ittaiz
Copy link
Member

ittaiz commented Jan 22, 2018

I think I wasn't clear.
I want the toolchain to only contain what you're suggesting and the scala version (2.10/2.11/2.12).
Then I suggest the *_repositories methods to use that when they depend on artifacts.
If we do that then we make sure people can use the default repositories with 2.11/2.12 toolchains and that if anyone wants a different one it's simply a matter of calling their own _repostiories method.
Am I missing something?

@johnynek
Copy link
Member Author

I don't think there is a way to get data out of the toolchain except in a skylark rule (not macro), since you can't access ctx.toolchains unless you are in a skylark rule.

so, i don't know a good way to store a version string for others to consume. Maybe we output it to a file?

i can add scala_major_version to the toolchain if you like. Is that what you mean @ittaiz ?

@ittaiz
Copy link
Member

ittaiz commented Jan 22, 2018 via email

@katre
Copy link
Member

katre commented Jan 23, 2018

You're right that macros don't see toolchains (macros are evaluated when the file is loaded, and that's long before a single toolchain is resolved).

If something needs to know the version, and the version is in the toolchain, then the "something" needs to be a full rule.

@katre
Copy link
Member

katre commented Jan 23, 2018

Going back to the first comment on this PR, are you actually considering adding four more types of toolchains, or are you adding four more types of data to the existing toolchain? I don't have a perference between these, I am just curious about the choices you're making so I can be sure that future toolchains work is taking real world usage into account.

@johnynek
Copy link
Member Author

@katre I don't think we know yet. Basically our challenge is this: pretty much every external jar needs to understand which scala-version we are at.

Using select might be useful here, but I guess select can't see the toolchain either?

An alternate idea: maybe scala_import can know about the toolchain, and it could get select-like features. Then if we make sure all inputs to scala rules are scala_import, maybe it can handle the multiplexing.

@ittaiz
Copy link
Member

ittaiz commented Jan 23, 2018

Oscar,
I had the same thought though I think scala_import is too late since it's not a repository rule.
I'm thinking of scala_import_external repository rule which will depend on the toolchain and expose scala_import targets.
@katre can a repository rule use toolchains?
In scala artifacts are distinguished by _majorVersion suffix in the artifactId like specs2_2.11 and specs2_2.12

@johnynek
Copy link
Member Author

@ittaiz but what if scala_import allowed you to import multiple versions?

Like:

scala_import(
  name = "foo",
  jar_map = {
    "jvm_2.11": { jar = "some_jar", sha256 = "... },
    "jvm_2.12": ...
  }
)

or something like this. Its a pain to write it out, but tooling could do it maybe.

@ittaiz
Copy link
Member

ittaiz commented Jan 23, 2018

why does scala_import need the map with the sha? in your example is it a repository rule or a "regular" rule?

@johnynek
Copy link
Member Author

@ittaiz I think I'm conflating them in my thinking.

@ittaiz
Copy link
Member

ittaiz commented Jan 23, 2018

I thought so :)
I think your above offer is somewhat problematic since it means duplicating the number of downloaded jars.
Let's wait for @katre's input about whether or not we can use the toolchain in a repository rule.
If we can't we'll need to go back to the drawing board

@johnynek
Copy link
Member Author

closes #380

@katre
Copy link
Member

katre commented Jan 24, 2018

Toolchains are also not available in repository rules.

Why does scala_import need the scala version? I'm afraid I don't fully understand this part of the scala rules.

@katre
Copy link
Member

katre commented Jan 24, 2018

Also, sorry for the delays in responses, I am in Europe this week so my response times are skewed.

@ittaiz
Copy link
Member

ittaiz commented Jan 24, 2018

sure thing, thanks for responding!
Scala, as a language, doesn't keep binary compatibility between minor versions. This means that if I need to depend on a library (for example scalaz) I need to depend on the version of it that was built with my version of scala.
The convention for this in scala is to add the version as a suffix to the artifact id of the library.
So if I use scala 2.11 I'll depend on scalaz_2.11 (note the artifact Id that changes). Conversely if I use scala 2.12 I'll depend on scalaz_2.12.
That means the actual maven coordinates differ between the scala versions I need to use which is exactly what we want to keep in the toolchain

@katre
Copy link
Member

katre commented Jan 25, 2018

I assume that people won't be using multiple scala versions in the same project. If they are, that gets much more difficult.

Based on that assumption, I'd put the scala version into the repository rule as an argument (as I see is proposed). The repository rule can write a BUILD file with a special target (scala_version?) that the toolchain and other rules like scala_import can depend on to learn the expected scala version.

Example WORKSPACE:

scala_repository(version="2.12")

scala_repository writes a BUILD file in @io_bazel_rules_scala/BUILD:

scala_version(name = 'version', version='2.12')

The scala version rule has a ScalaVersionInfo provider with the version.

Then toolchains and other scala rules can depend on @io_bazel_rules_scala//:version, get the ScalaVersionInfo provider, and read the current version string.

It's a little complicated, but would that allow defining the version in one place and accessing it from others?

@gkossakowski
Copy link

👍 to @katre's idea.
This is essentially saying that Scala version will be wired into bootstrapping bazel. I think makes sense given bazel's preference for static information/graphs wherever possible and the fact that Scala's version popups everywhere: including in bootstrapping scala rules as part of the loading phase.

1 similar comment
@gkossakowski
Copy link

👍 to @katre's idea.
This is essentially saying that Scala version will be wired into bootstrapping bazel. I think makes sense given bazel's preference for static information/graphs wherever possible and the fact that Scala's version popups everywhere: including in bootstrapping scala rules as part of the loading phase.

@ittaiz
Copy link
Member

ittaiz commented Jan 25, 2018

Sounds like it's our only current choice.
@katre ideally we'd like to be able to use multiple scala versions in the same repo (see #80) but your suggestion sounds like a good step forward. Is it feasible to repository rules to have access to toolchains?
I'm interested in @johnynek opinion as well :)

@katre
Copy link
Member

katre commented Jan 25, 2018

Unfortunately, repository rules are not part of the analysis phase and will not have access to toolchains. However, a repository rule can write a BUILD file with targets that are part of analysis and can have toolchains.

@ittaiz
Copy link
Member

ittaiz commented Jan 25, 2018

so in theory one might be able to say scala_repository(versions="2.11", "2.12"]) and then the repository rules will download binaries for both versions and defer the question of which to expose to a later point (where the toolchain can be used)

@katre
Copy link
Member

katre commented Jan 25, 2018

Yes, that would be possible.

@ittaiz
Copy link
Member

ittaiz commented Jan 25, 2018

ok. I think we have a winner ;)

@ittaiz
Copy link
Member

ittaiz commented Jan 26, 2018 via email

@johnynek
Copy link
Member Author

Yes, this sounds like a good plan.

@ittaiz ittaiz mentioned this pull request Feb 22, 2018
@pauldraper
Copy link

pauldraper commented Apr 6, 2018

Toolchains

This is essentially saying that Scala version will be wired into bootstrapping bazel.

This is precisely why I've not loved toolchains.

This approach is similar to sbt, for which scalaVersion can only be one value at a time. You would then run two separate versions of your build graph.

           A (2.11)
           |
C (2.11)   B (2.11)
     \   /
    E (2.11)
           A (2.12)
           |
           B (2.12)
            \ 
              F (2.12)

Explicitly specify in macros

In sbt-land, I wound up creating sbt-cross which duplicates sbt projects by scala version (basically, like a Bazel macro). That way, they all live in the same build graph, and I can build them concurrently, etc.

           A (2.11 + 2.12)
           |
C (2.11)   B (2.11 + 2.12)
     \   /   \ 
    E (2.11)  F (2.12)
// it essentially translates to the above, but is at least part of the same forest

Aspects

rules_typescript takes an approach where they use aspects to compute what versions (Ecmascript versions in that case) are needed by downstream targets. It seems complicated to me, but maybe there's something there.


I assume that people won't be using multiple scala versions in the same project.

This ties the entire workspace (and this is bazel, so repos/workspaces are large) to a single version. In my experience, the hardest part of upgrading is external dependencies that have not built for you the version you want. And when every project is held up by one, that is challenging.

@ittaiz
Copy link
Member

ittaiz commented Apr 7, 2018 via email

@katre
Copy link
Member

katre commented Apr 9, 2018

@ittaiz Sorry, I'm not sure what you mean by "the design" at this point (I admit I haven't been fully following this thread).

My understanding is that the major issue is with imported libraries, which implicitly depend on a specific scala version (due to binary incompatbility). Is this correct?

I would advise you to go with a design that serves your users now, and once more configurability options become available, we can work together on how to re-implement your rules using the new features.

@johnynek
Copy link
Member Author

replaced by #530

@johnynek johnynek closed this Jun 26, 2018
@ittaiz ittaiz deleted the oscar/toolchain-libs branch July 1, 2018 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants