Add scala-xml and scala-parser-combinators jars individually #549

jhnj · 2018-06-28T18:55:48Z

As part of a larger PR (#544) for adding support for selecting scala versions I separated scala-xml and scala-parser-combinators to be imported individually using native.http_jar. @johnynek suggested making o separate PR for this smaller change so here it is.

The motivation behind the change is that the versions of scala-xml and scala-parser-combinators that are downloaded as part of the main {scala_version}.tgz differ between minor scala versions and this way we can select specific versions (#544 would allow users to select any 2.11/2.12 minor version)

- Update aspect test with new labels

googlebot · 2018-06-28T18:55:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

johnynek · 2018-06-28T18:57:30Z

@jhnj you need to sign the CLA. Stripe has already done it for all of stripe. If you use your stripe account, it should work, or I think you can personally sign.

johnynek · 2018-06-28T18:58:55Z

scala/scala.bzl

-    jars = ["lib/scala-parser-combinators_2.11-1.0.4.jar"],
-    visibility = ["//visibility:public"],
-)
-
 java_import(


can we kill the whole use of the sdk and just use jars? I think that might be the way to go so we can just use maven servers for all dependencies (or git repos I guess).

Don't see why we couldn't, I'll update the PR after lunch

jhnj · 2018-06-28T19:04:24Z

I'll sign the cla individually. Decided to use my personal account as I won't have access to the stripe one after the summer

jhnj · 2018-06-28T20:19:19Z

I signed it!

googlebot · 2018-06-28T20:19:22Z

CLAs look good, thanks!

jhnj · 2018-06-28T20:21:53Z

@johnynek Updated the PR. One question though, currently using native.http_jar for the downloads (as scalatest was using it). That or native.maven_jar?

johnynek

move to http_file + scala_import and I am +1

johnynek · 2018-06-28T21:35:09Z

scala/scala.bzl

+      url =
+      "http://central.maven.org/maven2/org/scala-lang/modules/scala-parser-combinators_2.11/1.0.4/scala-parser-combinators_2.11-1.0.4.jar",
+      sha256 = "0dfaafce29a9a245b0a9180ec2c1073d2bd8f0330f03a9f1f6a74d1bc83f62d6"
+  )



I think we need to use http_file then scala_import them because they could in principle have macros inside, so we don't want to use ijar.

ittaiz · 2018-06-28T21:48:09Z

Please just use scala_import_external or better yet scala_maven_import_external They do the same but in a cleaner and more semantic fashion

…

On Fri, 29 Jun 2018 at 0:36 P. Oscar Boykin ***@***.***> wrote: ***@***.**** commented on this pull request. move to http_file + scala_import and I am +1 ------------------------------ In scala/scala.bzl <#549 (comment)> : > @@ -300,6 +278,18 @@ def scala_repositories(): sha256 = "f198967436a5e7a69cfd182902adcfbcb9f2e41b349e1a5c8881a2407f615962", ) + native.http_jar( + name = "scala_xml", + url = + "http://central.maven.org/maven2/org/scala-lang/modules/scala-xml_2.11/1.0.5/scala-xml_2.11-1.0.5.jar", + sha256 = "767e11f33eddcd506980f0ff213f9d553a6a21802e3be1330345f62f7ee3d50f" + ) + native.http_jar( + name = "scala_parser_combinators", + url = + "http://central.maven.org/maven2/org/scala-lang/modules/scala-parser-combinators_2.11/1.0.4/scala-parser-combinators_2.11-1.0.4.jar", + sha256 = "0dfaafce29a9a245b0a9180ec2c1073d2bd8f0330f03a9f1f6a74d1bc83f62d6" + ) I think we need to use http_file then scala_import them because they could in principle have macros inside, so we don't want to use ijar. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#549 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF9htHGRNyNh0vnrSgQfFun_GGcH8ks5uBUxGgaJpZM4U7562> .

jhnj · 2018-06-28T22:33:09Z

@johnynek @ittaiz now with http_file + scala_import. Didn't know about scala_maven_import_external, I'll use that

johnynek · 2018-06-28T23:06:46Z

can you look at the CI failures? seems like you still have some @scala// around.

jhnj · 2018-06-28T23:08:09Z

@johnynek Updated now with scala_maven_import_external, I'll check the CI failures

jhnj · 2018-06-28T23:14:12Z

Not sure what the CI failures where, didn't get those errors locally. We'll see if the new commits pass

jhnj · 2018-06-29T00:38:49Z

@johnynek Could you take a look at the CI errors, can't figure out why it's failing.

dependency_analyzer_test is failing as it's not able to resolve $(location //external:io_bazel_rules_scala/dependency/scala/scala_library). I couldn't get it working locally by changing to @scala_library//jar:jar either.

johnynek · 2018-06-29T00:56:06Z

@jhnj I see:

bazel test third_party/...

failing. Does that work for you?

jhnj · 2018-06-29T01:18:45Z

@johnynek Doesn't work, dependency_analyzer_test is in third_party. I'm getting:

ERROR: /Users/johan/code/rules_scala/third_party/plugin/src/test/BUILD:5:1: in scala_junit_test rule //third_party/plugin/src/test:dependency_analyzer_test: label '@scala_library//jar:jar' in $(location) expression expands to no files

(and the same with //external:io_bazel_rules_scala/dependency/scala/scala_library)

jhnj · 2018-06-29T01:21:41Z

@scala_library is added using scala_maven_import_external and works correctly elsewhere

johnynek · 2018-06-29T01:25:47Z

@ittaiz any tips? I have not really kept up with the state of these import rules.

ittaiz · 2018-06-29T05:33:17Z

I’ll take a look later today

…

On Fri, 29 Jun 2018 at 4:25 P. Oscar Boykin ***@***.***> wrote: @ittaiz <https://github.com/ittaiz> any tips? I have not really kept up with the state of these import rules. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#549 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF5e7ySrBdsX_PV-GcBZN2OD5UCFQks5uBYIcgaJpZM4U7562> .

ittaiz · 2018-06-29T06:08:27Z

I think, but haven’t confirmed it yet, that it’s because scala import doesn’t return the files in the DefaultInfo provider. Third time we’re stublin onto this but each time it’s part of a bigger PR so it doesn’t get fixed. I’ll see if I can send a small PR just for this. @jhnj if you’re blocked by it you can also pick it up

jhnj · 2018-06-29T17:20:19Z

I'll try to make a PR today if I have the time

jhnj · 2018-06-29T19:30:10Z

Cherry-picked commit from #550

johnynek · 2018-06-29T20:02:57Z

scala/scala.bzl

-    visibility = ["//visibility:public"],
-)
-"""
-
 def scala_repositories():


Can we make server URLs maven_servers as a parameter and default to a single list with maven central?

johnynek · 2018-06-29T20:22:32Z

scala/scala.bzl

+  )
+
+  _scala_maven_import_external(
+      name = "scala_xml",


These names are pretty short. Since they have to be globally unique what about io_bazel_rules_scala_scala_xml?

Doesn’t this contradict canonical naming? Why do you think here we should preface with rules_scala?
Not sure I disagree btw, trying to gather my thoughts about it

We don’t own the global namespace. So who is to say someone else didn’t point scala_xml at something else. If you prefix with io_bazel_rules_scala I think you are asking for it. I think we can argue that we do own that namespace. If we could solve the problem of a canonical maven encoding, this could be a solved problem.

👍 on having longer names. Don't have strong opinions on whether it should be io_bazel_rules_scala_scala_xml or e.g. org_scala_lang_modules_scala_xml

Add scala-xml and scala-parser-combinators individually

6a3a76f

- Update aspect test with new labels

googlebot added the cla: no label Jun 28, 2018

johnynek reviewed Jun 28, 2018

View reviewed changes

Download scalalib, compiler and reflect as jars

81f6e15

googlebot added cla: yes and removed cla: no labels Jun 28, 2018

johnynek reviewed Jun 28, 2018

View reviewed changes

Download jars using http_file, import using scala_import

18dbe06

Use

e7d88e6

Run linter

52d608c

jhnj added 2 commits June 28, 2018 16:23

Remove https from maven url

b363bc3

Correct artifacts

78c2aef

jhnj mentioned this pull request Jun 29, 2018

Add DefaultInfo provider to outputs of scala_import #550

Merged

Add DefaultInfo provider to outputs of scala_import

7e6e590

johnynek suggested changes Jun 29, 2018

View reviewed changes

johnynek reviewed Jun 29, 2018

View reviewed changes

jhnj added 2 commits June 29, 2018 13:42

maven_servers as parameter

da71a4c

Prefix scala_repositories labels with io_bazel_rules_scala

798fbf8

johnynek approved these changes Jun 29, 2018

View reviewed changes

johnynek merged commit 76d4ab9 into bazelbuild:master Jun 29, 2018

travisbrown mentioned this pull request Jul 3, 2018

Update to latest rules_scala, etc. bazeltools/bazel-deps#176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scala-xml and scala-parser-combinators jars individually #549

Add scala-xml and scala-parser-combinators jars individually #549

jhnj commented Jun 28, 2018

googlebot commented Jun 28, 2018

johnynek commented Jun 28, 2018

johnynek Jun 28, 2018

jhnj Jun 28, 2018

jhnj commented Jun 28, 2018

jhnj commented Jun 28, 2018

googlebot commented Jun 28, 2018

jhnj commented Jun 28, 2018

johnynek left a comment

johnynek Jun 28, 2018

ittaiz commented Jun 28, 2018 via email

jhnj commented Jun 28, 2018

johnynek commented Jun 28, 2018

jhnj commented Jun 28, 2018

jhnj commented Jun 28, 2018

jhnj commented Jun 29, 2018

johnynek commented Jun 29, 2018

jhnj commented Jun 29, 2018 •

edited

Loading

jhnj commented Jun 29, 2018

johnynek commented Jun 29, 2018

ittaiz commented Jun 29, 2018 via email

ittaiz commented Jun 29, 2018

jhnj commented Jun 29, 2018

jhnj commented Jun 29, 2018

johnynek Jun 29, 2018

johnynek Jun 29, 2018

ittaiz Jun 29, 2018

johnynek Jun 29, 2018

jhnj Jun 29, 2018

Add scala-xml and scala-parser-combinators jars individually #549

Add scala-xml and scala-parser-combinators jars individually #549

Conversation

jhnj commented Jun 28, 2018

googlebot commented Jun 28, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

johnynek commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhnj commented Jun 28, 2018

jhnj commented Jun 28, 2018

googlebot commented Jun 28, 2018

jhnj commented Jun 28, 2018

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz commented Jun 28, 2018 via email

jhnj commented Jun 28, 2018

johnynek commented Jun 28, 2018

jhnj commented Jun 28, 2018

jhnj commented Jun 28, 2018

jhnj commented Jun 29, 2018

johnynek commented Jun 29, 2018

jhnj commented Jun 29, 2018 • edited Loading

jhnj commented Jun 29, 2018

johnynek commented Jun 29, 2018

ittaiz commented Jun 29, 2018 via email

ittaiz commented Jun 29, 2018

jhnj commented Jun 29, 2018

jhnj commented Jun 29, 2018

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

jhnj commented Jun 29, 2018 •

edited

Loading