Keep all maven deps in the central place for easier version management #1113

liucijus · 2020-09-24T07:12:22Z

I've been thinking how to make external maven deps management simpler and decided to move all maven deps into single location per Scala version. I hope this will bring better visibility on which deps we are using and help update them more routinely.

My hope is that this refactoring helps towards:

an easier manual or automated update of versions
adding support of new Scala versions, eg Support Scala 2.13 #809
a unified user experience when consuming rules with deps from repository macros
less code duplication

This is a breaking change for some repository users (eg. scrooge). It's also not fully tested yet, marking PR as a draft and calling for your opinions.

blorente

In general, this looks really good and a change in a direction I really love to see, thank you so much!

I've left a comment regarding some implementation details on the scrooge side. Don't really mind breaking changes, as long as we can keep the functionality described in some form.

Let me know if I can help any more!

blorente · 2020-09-24T10:59:56Z

twitter_scrooge/twitter_scrooge.bzl

-        scrooge_generator = None,
-        util_core = None,
-        util_logging = None):
+        overriden_artifacts = {}):


If I understand this change correctly, this will work if what we want is to resolve a different version of the artifacts.

However, our use case (and the main reason we implemented the overrides) was that we want to replace them with source-targets, not artifacts, and avoid the resolve.

I think we could achieve that by modifying the for_artifact_ids list before passing it to repositories, removing everything that we give a target for.

@blorente Do you want to keep using override approach, or would you consider moving dependencies to a toolchain? I have a branch for scrooge already, but I will make a PR after other toolchains PRs will progress: https://github.com/liucijus/rules_scala/tree/scrooge-toolchain

Moving to a toolchain would be great, as long as we keep avoiding the resolution for targets that we override.

We'd be happy to migrate to toolchains, looking forward to that PR, we can discuss the specifics there :)

I guess, if this PR lands before the scrooge toolchains PR, we would be blocked on upgrading rules_scala until the scrooge toolchains PR lands, but I think that is not a big problem :)

I hope to get with the toolchain first, otherwise I will leave previous interface

ittaiz · 2020-09-25T05:15:26Z

build still fails

simuons · 2020-09-28T05:46:23Z

junit/junit.bzl

-        licenses = ["notice"],
-        server_urls = maven_servers,
+def junit_repositories(
+        maven_servers = _default_maven_server_urls(),


maven_servers is not passed to repositories

Thanks for catching, @simuons!

simuons · 2020-09-28T05:50:42Z

scala/scalafmt/scalafmt_repositories.bzl

-            extra_jar_version = scala_version_extra_jars["com_geirsson_metaconfig_typesafe_config"]["version"],
-        ),
-        artifact_sha256 = scala_version_extra_jars["com_geirsson_metaconfig_typesafe_config"]["sha256"],
+        maven_servers = _default_maven_server_urls(),


I think maven_servers should be taken from prams of scalafmt_repositories

simuons · 2020-09-28T05:54:27Z

scala_proto/private/scala_proto_default_repositories.bzl


 def scala_proto_default_repositories(
        scala_version = _default_scala_version(),
-        maven_servers = _default_maven_server_urls()):
+        maven_servers = _default_maven_server_urls(),
+        overriden_artifacts = {}):
    major_version = _extract_major_version(scala_version)


I think major_version is not passed to repositories

simuons · 2020-09-28T05:56:45Z

specs2/specs2.bzl


 def specs2_version():
    return "4.4.1"

 def specs2_repositories(
        scala_version = _default_scala_version(),
-        maven_servers = _default_maven_server_urls()):
+        maven_servers = _default_maven_server_urls(),
+        overriden_artifacts = {}):
    major_version = _extract_major_version(scala_version)


Maybe this major_version = _extract_major_version(scala_version) could be in repositories function? It would be possible to remove this duplicated line from other *_repositories

simuons

👍

or-shachar

Looks very good Vaidas!! I think once this is merged we should prioritize creating a base list and have some sort of tool that will generate those files automatically for all scala flavors.

or-shachar · 2020-09-30T12:57:16Z

third_party/repositories/repositories.bzl

+    "2.12": _artifacts_2_12,
+}
+
+def repositories(


The name repositories does not sound right, maybe I'm wrong. Let's continue reading and see

I understand why you chose repositories... To keep current naming convention - but maybe it's an opportunity to name it better? define_maven_external_dependencies or something similar?

I prefer shorter names, I think finding specific names with underscores is harder for users. I think name that includes repositories is right as it aligns with its only usecase repository bzl pattern. I don't expect any other usecases.

or-shachar · 2020-09-30T13:24:54Z

specs2/specs2.bzl

-        licenses = ["notice"],
-        server_urls = maven_servers,
+        scala_version = scala_version,
+        maven_servers = maven_servers,


If we're using the default servers all the time - why can't it be just the default value and skip the explicit parameter?

Users may provide other server urls

or-shachar · 2020-09-30T13:26:18Z

third_party/repositories/repositories.bzl

+def repositories(
+        scala_version = default_scala_version(),
+        for_artifact_ids = [],
+        maven_servers = default_maven_server_urls(),


Oh I see you already call here for default value - so can we remove the explicit parameter from caller site?

Users will need to be able to provide their own urls

or-shachar · 2020-09-30T13:28:57Z

third_party/repositories/scala_2_11.bzl

@@ -0,0 +1,416 @@
+artifacts = {


Awesome!! How did you create this list? Did you use some kind of generator?
What happens when the next dev wants to update something?

Currently it's a manual procedure to update deps

…ersion

…l, to address dep analyzer tests

liucijus · 2020-10-09T07:18:38Z

@blorente, as while we have discussion on #1116, I have restored existing Twitter Scrooge repositories interface. I would appreciate if you could take a look.
Otherwise this PR is in ready to be merged state.

blorente · 2020-10-09T12:54:21Z

@liucijus Looks good to me, thanks for putting so much thought into scrooge!

bazelbuild#1113) * Move external artifact versions and shas to central place per Scala version * Move scalafmt deps to repositories * Move jmh deps to repositories * Move junit deps to repositories * Move specs2 deps to repositories * Move specs2-junit deps to repositories * Move proto deps to repositories * Move tut deps to repositories * Move scrooge deps to repositories * Move WORKSPACE test deps to repositories files * Move dep back to workspace to explicitly use jvm_maven_import_external, to address dep analyzer tests * Fix Scala version tests * Use repository deps for scrooge * Add scrooge version tests back * Keep current twitter scrooge repository interface

googlebot added the cla: yes label Sep 24, 2020

blorente reviewed Sep 24, 2020

View reviewed changes

simuons reviewed Sep 28, 2020

View reviewed changes

simuons approved these changes Sep 28, 2020

View reviewed changes

liucijus mentioned this pull request Sep 29, 2020

Scala 2 13 #1118

Merged

or-shachar reviewed Sep 30, 2020

View reviewed changes

liucijus force-pushed the repositories-refactoring branch from 8cad240 to 70d00c6 Compare October 8, 2020 12:55

Vaidas Pilkauskas added 15 commits October 8, 2020 19:43

Move external artifact versions and shas to central place per Scala v…

155b209

…ersion

Move scalafmt deps to repositories

7568f85

Move jmh deps to repositories

f0397b9

Move junit deps to repositories

dc7652b

Move specs2 deps to repositories

2830f2c

Move specs2-junit deps to repositories

a1ea9c8

Move proto deps to repositories

19d7fcc

Move tut deps to repositories

82155d3

Move scrooge deps to repositories

7d347e2

Move WORKSPACE test deps to repositories files

5196a96

Move dep back to workspace to explicitly use jvm_maven_import_externa…

594dcf4

…l, to address dep analyzer tests

Fix Scala version tests

e129e97

Use repository deps for scrooge

a32755a

Add scrooge version tests back

89a41d1

Keep current twitter scrooge repository interface

bef58ac

liucijus force-pushed the repositories-refactoring branch from 70d00c6 to bef58ac Compare October 9, 2020 06:38

liucijus changed the title ~~Keep all maven deps in the central place foe easier version management~~ Keep all maven deps in the central place for easier version management Oct 9, 2020

liucijus marked this pull request as ready for review October 9, 2020 07:19

liucijus requested a review from ittaiz as a code owner October 9, 2020 07:19

ittaiz approved these changes Oct 11, 2020

View reviewed changes

ittaiz merged commit d8f5bd9 into bazelbuild:master Oct 11, 2020

liucijus mentioned this pull request Dec 2, 2020

Support different versions of scala #14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep all maven deps in the central place for easier version management #1113

Keep all maven deps in the central place for easier version management #1113

liucijus commented Sep 24, 2020

blorente left a comment

blorente Sep 24, 2020

liucijus Sep 24, 2020

blorente Sep 24, 2020

liucijus Sep 24, 2020

ittaiz commented Sep 25, 2020

simuons Sep 28, 2020

liucijus Sep 28, 2020

simuons Sep 28, 2020

simuons Sep 28, 2020

simuons Sep 28, 2020

simuons left a comment

or-shachar left a comment

or-shachar Sep 30, 2020

or-shachar Sep 30, 2020

liucijus Oct 1, 2020

or-shachar Sep 30, 2020

liucijus Oct 1, 2020

or-shachar Sep 30, 2020

liucijus Oct 1, 2020

or-shachar Sep 30, 2020

liucijus Oct 1, 2020

liucijus commented Oct 9, 2020

blorente commented Oct 9, 2020

Keep all maven deps in the central place for easier version management #1113

Keep all maven deps in the central place for easier version management #1113

Conversation

liucijus commented Sep 24, 2020

blorente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simuons left a comment

Choose a reason for hiding this comment

or-shachar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liucijus commented Oct 9, 2020

blorente commented Oct 9, 2020