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

Introduce Scala config #1133

Merged
merged 5 commits into from
Nov 6, 2020
Merged

Introduce Scala config #1133

merged 5 commits into from
Nov 6, 2020

Conversation

liucijus
Copy link
Collaborator

@liucijus liucijus commented Nov 5, 2020 •
edited
Loading

This is a breaking change!

To support multiple versions of Scala within rules codebase we need a way to check for current Scala version in rule and macro code. For example, to support Scala 2.13 properly we need to be able to pass different scalac opts to some targets. This will allow to do conditionals like this:

load("@io_bazel_rules_scala_config//:config.bzl", "SCALA_MAJOR_VERSION")

scala_library(
  scalacopts = [
     # 2.13 options
  ] if SCALA_MAJOR_VERSION == "2.13" else [ 
     # options for other versions
  ],
)

In addition, I moved bazel_version under scala_config to simplify things rules_scala users need to configure. It will allow us to add more configuration options with default values without breaking an existing user code.

How to upgrade:
In WORKSPACE file replace

load("@io_bazel_rules_scala//:version.bzl", "bazel_version")
bazel_version(name = "bazel_version")

with

load("@io_bazel_rules_scala//:scala_config.bzl", "scala_config")
scala_config()

or if non-default Scala version is in use, with explicit Scala version specified:

load("@io_bazel_rules_scala//:scala_config.bzl", "scala_config")
scala_config(scala_version = "2.11.12")

@liucijus liucijus requested a review from ittaiz as a code owner November 5, 2020 12:11
@google-cla google-cla bot added the cla: yes label Nov 5, 2020
simuons
simuons reviewed Nov 6, 2020
View reviewed changes
scala_config.bzl Outdated
return "2.12.11"

def _store_config(repository_ctx):
bazel_version = versions.get()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe replace versions.get() with native.bazel_version? This is what skylib does under the hood. As it is now it requires skylib to be declared before scala_config. Maybe not an issue though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, good point!

simuons
simuons reviewed Nov 6, 2020
View reviewed changes
README.md Outdated
@@ -50,8 +50,9 @@ http_archive(
sha256 = "8c48283aeb70e7165af48191b0e39b7434b0368718709d1bced5c3781787d8e7",
)

load("@io_bazel_rules_scala//:version.bzl", "bazel_version")
bazel_version(name = "bazel_version")
#
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually a mistake - I forgot to add documentation text here

simuons
simuons reviewed Nov 6, 2020
View reviewed changes
scala_config.bzl
},
)

def scala_config(scala_version = _default_scala_version()):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about scala_config naming. It contains only versions so maybe versions would be more appropriate. But as I understand it is a placeholder for other things as well. Maybe just config as it contains non scala related thing like BAZEL_VERSION? Either way I'm fine as it is currently. So you can just ignore this comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have intentionally named it config to allow other configuration to be stored in the future.

ittaiz
ittaiz reviewed Nov 6, 2020
View reviewed changes
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.

Maybe add a comment with a snippet on the PR with what people need to do to upgrade?

WORKSPACE
@@ -177,7 +171,7 @@ jvm_maven_import_external(
name = "org_typelevel__cats_core",
artifact = scala_mvn_artifact(
"org.typelevel:cats-core:0.9.0",
default_scala_major_version(),
SCALA_MAJOR_VERSION,
Copy link
Member

Choose a reason for hiding this comment

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

Why the change from method to const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a reason to wrap the const inside a function? The default version functions are removed, ant the stored version should be a single source of truth.

@liucijus
Copy link
Collaborator Author

liucijus commented Nov 6, 2020

Maybe add a comment with a snippet on the PR with what people need to do to upgrade?

Done

@ittaiz ittaiz merged commit 72c27ef into bazelbuild:master Nov 6, 2020
@katre
Copy link
Member

katre commented Nov 22, 2020

The directions for project setup in README.md are now wrong: the suggested version a2f5852 does not contain scala_config.bzl.

This makes it pretty confusing for even a very savvy Bazel user to get started with rules_scala.

@ittaiz
Copy link
Member

ittaiz commented Nov 22, 2020

Thanks! Is it only the version that's wrong? cc @liucijus

@katre
Copy link
Member

katre commented Nov 23, 2020

I updated to head and fixed the sha, and it seemed to work.

Have any good scala tutorials to point me at? :)

@liucijus liucijus mentioned this pull request Nov 23, 2020
Merged
@ittaiz
Copy link
Member

ittaiz commented Nov 23, 2020

It's a big question and I really don't know why you need it and how much time you want to allocate :)
Small
https://twitter.github.io/scala_school/
http://twitter.github.io/effectivescala/
Bigger
https://www.handsonscala.com/ (this is a book many colleagues said was helpful to dive into scala by real code and topics)
https://www.udemy.com/course/rock-the-jvm-scala-for-beginners/
https://courseware.epfl.ch/courses/course-v1:EPFL+progfun1+2018_T1/about

Some Scala exercises (like coding katas) https://www.scala-exercises.org/std_lib/asserts

Hope it helps

@katre
Copy link
Member

katre commented Nov 23, 2020

Thanks! I've been wanting to learn, so this year I'm planning to do https://adventofcode.com/ in Scala as a forcing function.

@ittaiz
Copy link
Member

ittaiz commented Nov 23, 2020

Awesome! Welcome aboard to our community :)
Good luck and have fun...

blorente pushed a commit to twitter-forks/rules_scala that referenced this pull request Nov 26, 2020
Introduce Scala config (bazelbuild#1133)
40a667e 
* Add scala config repository to store global configuration options

* Fix example workspace

* Add docs

* Get bazel version directly

* Remove unused skylib load
@tanishiking tanishiking mentioned this pull request Oct 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simuons simuons simuons approved these changes

@ittaiz ittaiz ittaiz approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liucijus @katre @ittaiz @simuons