Migrate jar dependencies of tests from maven_jar to jvm_maven_import_external #871

jin · 2019-11-04T16:41:01Z

Description

This pull request converts all maven_jar usages to jvm_maven_import_external.

See bazelbuild/bazel#6799 for more information about maven_jar's removal.

Motivation

maven_jar is planned for removal in Bazel 2.0.

johnynek

I'm personally -1 on this.

I'd rather use http_jar (a custom implementation in this repo is only a few lines) and just point to the locations rather than add another dependency under all our users.

Secondly, the usability of pointing to paths with full URLs in them seems bad to me.

Lastly, the regression of including the scala version in the targets, I think will at least create large diffs when we do upgrades, and may significantly complicate things (e.g. it becomes easier to have 2_11 and 2_12 on the classpath, which creates very hard to debug errors for most users).

WORKSPACE

johnynek · 2019-11-04T17:16:39Z

test/BUILD

@@ -63,7 +63,7 @@ scala_binary(
 scala_library(
    name = "HelloLib",
    srcs = ["HelloLib.scala"],
-    plugins = ["@org_psywerx_hairyfotr__linter//jar"],
+    plugins = ["@maven//:org_psywerx_hairyfotr_linter_2_11"],


I'm still bummed you all didn't find an injective naming scheme. This can collide if you have org.bar:foo.baz and org.bar.foo:baz. That's a shame. The extra _ was to separate the org from the group.

Next, since you all are baking the scala version 2_11 into the targets people use, this will mean upgrading a repo will be a pretty major pain. It may be that every string ending in _2_11" can be converted to _2_12" but previously, we wouldn't have that giant diff. This is a regression in usability for scala users.

johnynek · 2019-11-04T17:17:38Z

test/src/main/scala/scalarules/test/scala_import/BUILD

-        "@com_google_guava_guava_21_0_with_file//jar:file",
-        "@org_apache_commons_commons_lang_3_5//jar:file",
+        "@maven//:v1/https/jcenter.bintray.com/com/google/guava/guava/21.0/guava-21.0.jar",
+        "@maven//:v1/https/jcenter.bintray.com/org/apache/commons/commons-lang3/3.5/commons-lang3-3.5.jar",


do we really need to use the source of the binary in the name? What about users that use private internal resolvers?

Filed bazel-contrib/rules_jvm_external#284

There's currently the ability to reference maven_install artifacts with the //jar-style labels with generate_compat_repositories, but we didn't implement the file label. We can definitely do that - will look into it very soon.

jjudd · 2019-11-04T17:29:12Z

I'm a fan of rules_scala either adopting the bazelbuild sanctioned third party jvm deps solution (rules_jvm_external) or contributing to it, such that it works for bazelbuild/rules_scala.

I'm personally +1 on moving to rules_jvm_external as we've been using rules_jvm_external for several months internally and our open source projects and it's been working well for us.

borkaehw · 2019-11-04T18:12:48Z

I like how easy to use rules_jvm_external. @jin has done hard works to make it great. The repo maintainers are very active, and I believe we can address the concerns (mentioned above) quickly and efficiently.

I am personally +1 to this and excited to see bazelbuild/rules_scala embrace more tightly to Bazel ecosystem.

johnynek · 2019-11-04T18:44:29Z

I'm sorry I responded to the review. @jin I'm no longer involved with the project so perhaps you want to disregard my opinion. I'll remove my name and unsubscribe from the notifications.

ittaiz · 2019-11-04T19:38:41Z

Hi,
I'm swamped with the beheamoth (#865) but I'll try to review it this week.
In general I have to say I don't like rules_jvm_external very much since it feels mostly relevant for small builds (I'd rather not open the whole discussion right now but if people strongly request it I will).
Not sure why not just use jvm_import_external like we do in most places.
A more detailed review (and to read what others have written so far) will happen later

jin · 2019-11-04T23:19:31Z

@johnynek thanks for the review! Very good comments and issues raised. These are definitely things that we can fix upstream in rules_jvm_external.

@ittaiz what about maven_install makes you feel that it's only meant for small builds? Certainly, I can see how that's the case if you use it without artifact pinning / maven_install.json.

I don't mind replacing rules_jvm_external with jvm_import_external in this PR. The goal is to move away from maven_jar, anyway.

borkaehw · 2019-11-14T17:39:39Z

Any update on this?

jin · 2019-11-14T17:58:23Z

I've switched rules_jvm_external to jvm_maven_import_external, which is a much smaller and less invasive change. PTAL.

borkaehw · 2019-11-14T18:24:19Z

I see. It's sad to see rules_jvm_external goes. 😢

ittaiz

Thanks a lot for this @jin!

ittaiz · 2019-11-15T13:51:06Z

@jin regarding rules_jvm_external this is a long discussion which I don't have the capacity to have right now, sorry. Late responding to this PR wasn't me trying to make a point but rather a pure capacity and prioritization issue (whatever time I have I try to put into the phases PR).

Hopefully we'll be able to talk about it at BazelCon :)

ittaiz · 2019-11-15T14:33:53Z

lastly the build is failing. I've checked out the branch and am trying to fix the tests

ittaiz · 2019-11-18T18:51:53Z

just to update, I was able to get past the failures in travis but got stuck on other failures and ran out of time. Will try to find more time as soon as I can

jin · 2019-11-27T22:32:49Z

@ittaiz I'm adding the @repo//jar:file target to jvm_maven_import_external to keep the API consistent with maven_jar, so the only changes to this PR should only be for the WORKSPACE file.

Unfortunately this will only land in Bazel 2.0. Perhaps we can extract the Starlark rules out to rules_jvm_external to decouple release processes.

I'll open another PR as soon as we have a jvm_maven_import_external that's API compatible to migrate rules_scala.

bazelbuild/bazel#10324

ittaiz · 2019-11-28T04:14:15Z

Thanks! Is it only for us or are others having a harder time to migrate? If it’s only for us please don’t add it. I’ll fix the tests here.

On Thu, 28 Nov 2019 at 0:39 Jin ***@***.***> wrote: Closed #871 <#871>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#871?email_source=notifications&email_token=AAKQQF7ZTE4RZBA4T5WHUALQV3ZJXA5CNFSM4JIWGFH2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOVEOBXDY#event-2837191567>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQFZX6S3TAP6VWF3X4S3QV3ZJXANCNFSM4JIWGFHQ> .

-- *Ittai Zeidman* Cell: 054-6735021 40 Hanamal street, Tel Aviv, Israel <http://www.wix.com>

googlebot · 2019-11-28T04:54:35Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

jin · 2019-11-28T04:56:12Z

Argh, my local git tree is so messed up. I'll make another PR.

Is it only for us or are others having a harder time to migrate? If it’s only for us please don’t add it.

Your use case is fairly common, so I think it's a good idea to add it.

ittaiz · 2019-11-28T06:29:38Z

I always prefer having less work so I won’t argue too much but I’ll just add that I think we used this pattern before we had the generic jvm_import_external and in many ways exposing the raw file is redundant since it widens the API. If people have other use cases then maybe you’re right

On Thu, 28 Nov 2019 at 6:56 Jin ***@***.***> wrote: Closed #871 <#871>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#871?email_source=notifications&email_token=AAKQQF2SQO3L3T7B4QLEPJLQV5FO7A5CNFSM4JIWGFH2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOVERYSHQ#event-2837678366>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF7RPLBOQ7RQIF46L5TQV5FO7ANCNFSM4JIWGFHQ> .

-- *Ittai Zeidman* Cell: 054-6735021 40 Hanamal street, Tel Aviv, Israel <http://www.wix.com>

…rt_external This adds compatibility between maven_jar and the Starlark rules' API, and helps with migration from maven_jar as seen in bazelbuild/rules_scala#871 Closes #10324. PiperOrigin-RevId: 282930614

jin requested a review from johnynek as a code owner November 4, 2019 16:41

googlebot added the cla: yes label Nov 4, 2019

jin mentioned this pull request Nov 4, 2019

Disable native.maven_jar: flip --incompatible_remove_native_maven_jar bazelbuild/bazel#10090

Closed

johnynek reviewed Nov 4, 2019

View reviewed changes

jin force-pushed the rm_maven_jar branch from 68d8236 to 35e32d7 Compare November 4, 2019 23:07

jin force-pushed the rm_maven_jar branch from 35e32d7 to 155a7e6 Compare November 14, 2019 17:57

jin requested a review from ittaiz as a code owner November 14, 2019 17:57

jin changed the title ~~Migrate jar dependencies of tests from maven_jar to rules_jvm_external~~ Migrate jar dependencies of tests from maven_jar to jvm_maven_import_external Nov 14, 2019

ittaiz approved these changes Nov 15, 2019

View reviewed changes

jin force-pushed the rm_maven_jar branch from ef71601 to 2504b90 Compare November 27, 2019 22:23

jin mentioned this pull request Nov 27, 2019

Add @repo//jar:file target to java_import_external and jvm_maven_import_external bazelbuild/bazel#10324

Closed

jin closed this Nov 27, 2019

jin force-pushed the rm_maven_jar branch from 2504b90 to 0f89c21 Compare November 27, 2019 22:39

chenrui333 and others added 4 commits November 28, 2019 06:15

Bump scala 2.12 to v2.12.10 (bazelbuild#886)

e568402

Convert maven_jar to jvm_maven_import_external

1d51442

Add artifact_sha256

6697903

Fix test_expect_failure/scala_import/BUILD

c83b0ad

buildifier WORKSPACE

2dd01f9

jin reopened this Nov 28, 2019

googlebot added cla: no and removed cla: yes labels Nov 28, 2019

jin closed this Nov 28, 2019

jin mentioned this pull request Nov 28, 2019

WIP: Replace maven_jar with maven_install #887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate jar dependencies of tests from maven_jar to jvm_maven_import_external #871

Migrate jar dependencies of tests from maven_jar to jvm_maven_import_external #871

jin commented Nov 4, 2019 •

edited

Loading

johnynek left a comment

johnynek Nov 4, 2019

johnynek Nov 4, 2019

jin Nov 4, 2019

jjudd commented Nov 4, 2019

borkaehw commented Nov 4, 2019 •

edited

Loading

johnynek commented Nov 4, 2019

ittaiz commented Nov 4, 2019

jin commented Nov 4, 2019 •

edited

Loading

borkaehw commented Nov 14, 2019

jin commented Nov 14, 2019

borkaehw commented Nov 14, 2019

ittaiz left a comment

ittaiz commented Nov 15, 2019

ittaiz commented Nov 15, 2019

ittaiz commented Nov 18, 2019

jin commented Nov 27, 2019 •

edited

Loading

ittaiz commented Nov 28, 2019 via email

googlebot commented Nov 28, 2019

jin commented Nov 28, 2019

ittaiz commented Nov 28, 2019 via email

Migrate jar dependencies of tests from maven_jar to jvm_maven_import_external #871

Migrate jar dependencies of tests from maven_jar to jvm_maven_import_external #871

Conversation

jin commented Nov 4, 2019 • edited Loading

Description

Motivation

johnynek left a comment

Choose a reason for hiding this comment

johnynek Nov 4, 2019

Choose a reason for hiding this comment

johnynek Nov 4, 2019

Choose a reason for hiding this comment

jin Nov 4, 2019

Choose a reason for hiding this comment

jjudd commented Nov 4, 2019

borkaehw commented Nov 4, 2019 • edited Loading

johnynek commented Nov 4, 2019

ittaiz commented Nov 4, 2019

jin commented Nov 4, 2019 • edited Loading

borkaehw commented Nov 14, 2019

jin commented Nov 14, 2019

borkaehw commented Nov 14, 2019

ittaiz left a comment

Choose a reason for hiding this comment

ittaiz commented Nov 15, 2019

ittaiz commented Nov 15, 2019

ittaiz commented Nov 18, 2019

jin commented Nov 27, 2019 • edited Loading

ittaiz commented Nov 28, 2019 via email

googlebot commented Nov 28, 2019

jin commented Nov 28, 2019

ittaiz commented Nov 28, 2019 via email

jin commented Nov 4, 2019 •

edited

Loading

borkaehw commented Nov 4, 2019 •

edited

Loading

jin commented Nov 4, 2019 •

edited

Loading

jin commented Nov 27, 2019 •

edited

Loading