Scala maven import external

[natansil]

Currently, there are limitations to using maven_jar + scala_import:

1. artifact information (e.g. version) and dependency information are separated between repository rules and build rules.
2. for cross-repo transitive dependencies there is the unintended consequences of trying to fetch the wrong transitive dependencies:
   Let's say we have repo-a and repo-b.
   repo-a depends on target foo from repo-b
foo depends on bar-v1
repo-a declares bar with v2
   bar-v1 brings baz
bar-v2 no longer depends on baz
   with the current design, when bazel builds foo for repo-a, it looks in repo-a’s workspace and fetches bar-v2
   but it will look in build file of repo-b for dependencies of bar and will try to fetch baz and will fail (because bazis not declared in workspace of repo-a )

In order to solve these issues a new repository rule should be defined that will also declare the transitive dependencies. This way the dependencies will be fetched from the original repo and not from external repos.

The idea is to extend the work done by @jart for java imports

Optimally, in order to have as much code reuse as possible, we want to strive to have as much generic code as possible with a jvm_import_external (see original proposal here) template rule that will allow to use it with either java_import or scala_import.

For now, we will work on implementing scala_maven_import_external based off of java_import_external rule and change it in such a manner as to enable easy migration to jvm_import_external .

jvm_import_external will include params:
rule_name (e.g. java_import, scala_import)
load (load statement, e.g. load("//scala:scala_import.bzl", "scala_import"))
use_ijar (propagate args to underlying rules. This case scala_import )
sha256 - make it optional
maven - allow translation from artifact to jar url ( maybe should actually reside in maven_import_external rule)

[johnynek]

are these alternative urls for the same sha?

[jart]

I can mirror files if you need it. Not sure I follow.

[johnynek]

is this just a temporary copy paste? can we not get this with a remote dependency?

[jart]

I can make the existing java_import_external serve your use case, if you'd prefer.

[ittaiz]

Yes this is a temporary copy paste.
We're starting out this way to validate the differences are indeed small instead of iterating between this repository and bazel itself.
The general plan is to generalize with as few changes as possible and then port 80%-90% of the code back to bazel

[ittaiz]

@natansil I don't have time now to do a complete review but WDYT about sharing here a bit more detail on what problem we're trying to solve and what are the gains we're trying to achieve? Also maybe a bit on intermediate solution vs longer term

[natansil]

@ittaiz updated the description along the lines we discussed.
@johnynek I hope the new description will shed more light about what we are trying to achieve here.

[natansil]

@ittaiz re-posting my question right next to the example of the consumer:

currently java_import_external does not support depending on the jar file directly. in order to not do major changes to justine's code, I've had to change the usage label structure from @com_google_guava_guava_21_0//jar:file to @com_google_guava_guava_21_0//:file.
Is this legitimate or should I re-write the code to have the same folder structure as maven_jar?

Alternatively, you've stated that scala_import can depend on full jar files without needing the file filegroup target.

so with the example above, I can just change it to
jars = ["@com_google_guava_guava_21_0//jar", "@org_apache_commons_commons_lang_3_5//jar"],

and that will solve the issue (and still not take ijars)?

[natansil]

@ittaiz , @johnynek please review.
The changes made:

• make the repo_rule write scala_import instead of java_import
• introduce artifact + server
• discuss possibility to extract a common "template" repo rule out of java_import_external and scala_maven_import_external

[jjudd]

We at Lucid are currently working on the external dependency issue and were just about to implement a scala_import_external by extending java_import_external so this is a very well timed PR :) Thanks for your work on this!

I have a couple questions:

The original java_import_external uses jar_urls instead of server_urls and artifact. Do we want to use jar_urls here? My thought is that we could use bazel-deps to resolve the maven dependencies, find the jar_urls and then produce a scala_import_external.

It seems like using bazel-deps would be quite nice: you can specify your dependencies in a file somewhere. Then import rules for those dependencies and all their transitive dependencies are generated.

This approach also works well with strict deps and unused deps. By using scala_import and java_import the generated import rules can declare their dependencies as deps instead of runtime_deps or exports.

How challenging would it be to create a generic jvm_import_external, which would be used to create java_import_external and scala_import_external? I'm hoping we could wrap java_import_external, as it is now, but pass in the rule you want it to produce: either java_import or scala_import.

If we create the jvm_import_external, do people want to use bazel-deps or is there a plan for Bazel to do something with Maven style dependencies without the need for a separate bazel-deps binary.

[natansil]

@jjudd,

Regarding 1. + 2.

As I see it this is part of the modularity effort we want to achieve.
At the base will be jvm_import_external that accepts the underlying import rule name (java_import or scala_import or foo_import) and also special args these rules needs.
It will also still have jar_urls attr, as a more basic feature.

In addition we are thinking about something like a maven_import_external that has artifact arg that can potentially wrap jvm_import_external and resolve the jar urls.
Hopefully these rules will be able to be part of bazel repo itself.

Regarding 3. I think that Bazel's long-term goal is to have transitive deps resolution built-in, like explained here

[googlebot]

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 cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[ittaiz]

I consent to my contributions going in

[natansil]

extract common code to jvm_import_external rule. introduce macros for scala_maven_import_external,
'maven_import_external', (maven_import_external could in a later stage delegates to
jvm_maven_import_external with maven_import rule which has MavenCoordinates provider - this way artifacts can easily be known by consumers)
'java_import_external' (for regression)

jvm_import_external now allows optional load statements and any other additional attributes the underlying import rule needs
I still need to change the documentation.

[ittaiz]

this should probably be an attribute with a default value since some organizations (like us) add these in prelude and also some organizations (stripe?) might have aliases or stuff so they load from a different place

[natansil]

done

[ittaiz]

i don't understand this (even with the comment sorry!)

[natansil]

changed a more detailed explanation. is this clear now?

[jjudd]

Thanks for generalizing things into jvm_import_external. Can we add a scala_import_external that looks like java_import_external, i.e., it takes jarl_urls, jar_sha256, srcjar_urls, and srcjar_sha256?

I ask because I imagine not everyone will want to rely on scala_maven_import_external to do the maven resolution, especially considering that the rule doesn't support transitive dependencies right now.

I'm currently modifying bazel-deps to use java_import_external and scala_import_external (an internal version) instead of java_libraryandscala_import. bazel-deps resolves the jars, finds all the information it needs, and then writes the rule calls to a .bzl` file.

I'm happy to add the rule to this PR. I just wanted to get people's feedback.

[ittaiz]

Did you see what kind of maven resolution the rule currently does? Why would you rather work with full URLs and not coordinates?

…

On Wed, 11 Apr 2018 at 18:59 James Judd ***@***.***> wrote: Thanks for generalizing things into jvm_import_external. Can we add a scala_import_external that looks likejava_import_external, i.e., it takes jarl_urls, jar_sha256, srcjar_urls, and srcjar_sha256? I ask because I imagine not everyone will want to rely on scala_maven_import_external to do the maven resolution, especially considering that the rule doesn't support transitive dependencies right now. I'm currently modifying bazel-deps to use java_import_external and scala_import_external (an internal version) instead ofjava_libraryandscala_import. bazel-deps resolves the jars, finds all the information it needs, and then writes the rule calls to a.bzl` file. I'm happy to add the rule to this PR. I just wanted to get people's feedback. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#473 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF3VAiL4wiEWxyNKSEJc91aSVoBuKks5tnihpgaJpZM4TG3ig> .

[jjudd]

I have read through the code. I still see value in having a scala_import_external that is used in the same way as java_import_external. I would like to separate Maven resolution from artifact downloading until the Bazel community agrees on a Maven resolution solution.

Maven resolution is a pretty active area in Bazel without an agreed on solution. I know of bazel-deps, transitive_maven_jar, native maven_jar, skylark maven_jar, and rules_maven. This adds another option to that mix. I would like to be able to use scala_import_external without subscribing to a particular artifact resolution strategy.

You can implement scala_maven_import_external using scala_import_external more easily than the other way around. The current commit does this for jvm_maven_import_external and jvm_import_external:

def jvm_maven_import_external(artifact, server_urls, **kwargs):
  jvm_import_external(
      jar_urls = _convert_to_url(artifact, server_urls),
      **kwargs
  )

I'm suggesting that we have the same thing for Scala.

[ittaiz]

I know the change is small, I just don’t understand the value. I don’t consider simple translation from coordinates to url as maven resolution but I don’t object if you plan to capitalize on it. @natansil, Can you add it?

…

On Wed, 11 Apr 2018 at 21:08 James Judd ***@***.***> wrote: I have read through the code. I still see value in having a scala_import_external that is used in the same way as java_import_external. I would like to separate Maven resolution from artifact downloading until the Bazel community agrees on a Maven resolution solution. Maven resolution is a pretty active area in Bazel without an agreed on solution. I know of bazel-deps, transitive_maven_jar, native maven_jar, skylark maven_jar, or rules_maven. This adds another option to that mix. I would like to be able to use scala_import_external without subscribing to a particular artifact resolution strategy. You can implement scala_maven_import_external using scala_import_external more easily than the other way around. The current commit does this for jvm_maven_import_external and jvm_import_external: def jvm_maven_import_external(artifact, server_urls, **kwargs): jvm_import_external( jar_urls = _convert_to_url(artifact, server_urls), **kwargs ) I'm suggesting that we have the same thing for Scala. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#473 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF-rBJYzqMdiFAIkANJ6YwOyCT6Qoks5tnkZ1gaJpZM4TG3ig> .

[natansil]

added scala_import_external
@jjudd

[ittaiz]

Looks good!
@johnynek any objections to this being merged?
Next step will be to open an issue to bazel linking to this code and suggesting we send it upstream (without the scala macros obviously)

[jjudd]

Thanks so much for adding scala_import_external @natansil and @ittaiz!! I used it in our bazel-deps work today and so far things are working great! 😃

[ittaiz]

@johnynek I'll merge in a couple of hours if you don't ping back