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

notable breaking change after upgrading #526

Closed
softprops opened this issue Jun 19, 2018 · 23 comments
Closed

notable breaking change after upgrading #526

softprops opened this issue Jun 19, 2018 · 23 comments

Comments

@softprops
Copy link
Contributor

I recently upgraded from 3f0b442 to 82b5b0e and ran into some issues which potentially surfaces the usefulness of having a CHANGELOG.md or other file for highlighting changes

I use a combination of both scala_library and scala_test rules from this project.

I picked up on this new requirement

load("@io_bazel_rules_scala//scala:toolchains.bzl", "scala_register_toolchains")
scala_register_toolchains()

By for some reason scala std lib seems not to be on the path for mixed compilation where java used to be able to import scala std lib types

Here's a tiny example where I used to be able to refer to scala.{Function,Unit} types but am no longer able to do so.

base/src/main/java/com/meetup/base/util/Shutdown.java:39: error: package scala does not exist
	public static void offer( final String name, scala.Function0<scala.Unit> fHook) {
	                                                  ^
base/src/main/java/com/meetup/base/util/Shutdown.java:39: error: package scala does not exist
	public static void offer( final String name, scala.Function0<scala.Unit> fHook) {

My best guess is this was a change to the way classpaths are managed. Before I go hunting though all the changes, does anyone here have some quick advise for a remedie?
@softprops
Copy link
Contributor Author

the notable way to see the changes ( and there's a lot of them ) is 3f0b442...82b5b0e

@ittaiz
Copy link
Member

ittaiz commented Jun 19, 2018 via email

@softprops
Copy link
Contributor Author

softprops commented Jun 19, 2018 •
edited
Loading

Do we want scala std lib to be on the classpath of java libraries

To clarify, these are scala_libraries which historically have relied the existing scala rule's scalac support for mixed compilation. That support seems now broken.

Cyclic dependencies shouldn't inherently be a result of mixed compilation. Scala's stdlib would be a common dependency for mixed compilation, not a cyclic one.

In practice, supporting mixed compilation very useful for evolving repositories. Use cases are java/scala interop are very common in the scala universe. Examples are java sources that require interfacing with scala apis which then depend on scala standard library types i.e. scala.Option ect and exporting apis from java that are more scala-friendly.

@softprops
Copy link
Contributor Author

Is there a recommendation to emulating the previous scala rules mixed compilation support in newer versions?

@johnynek
Copy link
Member

This sounds like a bug. We should add a java test (which we have) that exercises using the scala standard library. Then fixing the issue shouldn’t be very hard. If you have time, I’m happy to help point you to where we should make changes.

@softprops
Copy link
Contributor Author

I think the big change for this happened around 1da7e63

@softprops
Copy link
Contributor Author

@johnynek Context for exploration is my team's now coming back to the question of code coverage story which my team is starting to investigate. I'm now realizing that we're quite behind! We're on from bazel 0.6.1 ( latest is 0.14.1 ) and also somewhat behind up scala rules changes. I'm time boxing this current exploration but I think I can manage a pr with a failing test.

@softprops
Copy link
Contributor Author

what bazel version does the test harness expect to run on? I'm seeing this when I try test_all.sh locally

bazel version
Build label: 0.14.1
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri Jun 8 12:17:35 2018 (1528460255)
Build timestamp: 1528460255
Build timestamp as int: 1528460255
./test_all.sh
running test bazel build test/...

Log:

Starting local Bazel server and connecting to it...
.........
INFO: Options provided by the client:
  Inherited 'common' options: --isatty=0 --terminal_columns=80
INFO: Reading rc options for 'build' from /home/doug/code/meetup/rules_scala/tools/bazel.rc:
  'build' options: --experimental_local_disk_cache --experimental_local_disk_cache_path=.bazel_cache
ERROR: Unrecognized option: --experimental_local_disk_cache
 Test "bazel build test/..." failed  (1 sec)

@softprops
Copy link
Contributor Author

meh, I can thing work around that commenting out those options in tools/bazel.rc

@softprops
Copy link
Contributor Author

softprops commented Jun 19, 2018 •
edited
Loading

@johnynek would it be problematic that the test hardness for mixed compilation defines types in a package named scala?

@softprops
Copy link
Contributor Author

even without the bazel.rc flags the test_all.sh script seems to have some problems for me when running on 0.14.1. There seems to be other changes related to skylark type FileType. This seems to have changed in 0.14.0

./test_all.sh
running test bazel build test/...

 Test "bazel build test/..." successful (369 sec)
running test bazel build test/... --all_incompatible_changes

Log:

Loading:
Loading: 0 packages loaded
ERROR: /home/doug/.cache/bazel/_bazel_doug/558eb0e9f42eccd6693e510d527b5029/external/io_bazel_rules_scala/scala/private/rule_impls.bzl:28:18: FileType function is not available. You may use a list of strings instead. You can temporarily reenable the function by passing the flag --incompatible_disallow_filetype=false
ERROR: /home/doug/.cache/bazel/_bazel_doug/558eb0e9f42eccd6693e510d527b5029/external/io_bazel_rules_scala/scala/private/rule_impls.bzl:29:19: FileType function is not available. You may use a list of strings instead. You can temporarily reenable the function by passing the flag --incompatible_disallow_filetype=false
ERROR: /home/doug/.cache/bazel/_bazel_doug/558eb0e9f42eccd6693e510d527b5029/external/io_bazel_rules_scala/scala/private/rule_impls.bzl:30:20: FileType function is not available. You may use a list of strings instead. You can temporarily reenable the function by passing the flag --incompatible_disallow_filetype=false
ERROR: error loading package '': Extension file 'scala/private/rule_impls.bzl' has errors
ERROR: error loading package '': Extension file 'scala/private/rule_impls.bzl' has errors
INFO: Elapsed time: 0.262s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
FAILED: Build did NOT complete successfully (0 packages loaded)
 Test "bazel build test/... --all_incompatible_changes" failed  (1 sec)

@softprops
Copy link
Contributor Author

looks like the travis build is set for 0.13.0. consider this foreshadowing :)

@johnynek
Copy link
Member

you can comment out --all_incompatible_changes. That flag makes supporting multiple versions of bazel very hard sadly...

@johnynek
Copy link
Member

and yeah, that package name is unfortunate. Feel free to change it.

@ittaiz
Copy link
Member

ittaiz commented Jun 19, 2018 via email

@johnynek
Copy link
Member

Well, I think realistically, few people stay on the bazel upgrade treadmill (e.g. Doug is on 0.6.1). Google doesn't really have to care about keeping users, I actually would love more contributors to this project. Making it hard to use impedes that, in my view.

That said, maybe we should just add a compatibility.md file and keep a file of the last git sha that worked for each version and develop head for the most recently released version.

We do have tech debt, but it is hard to settle since fixing it while still allowing all our repos to build is hard. I'm still not on the build team, but I did spend basically all of last week on build, and I couldn't solve what we needed to solve. I think we need to grow the contributors.

@ittaiz
Copy link
Member

ittaiz commented Jun 19, 2018 via email

@softprops
Copy link
Contributor Author

My team and I hard going to take a side step for a bit so I may not get to following up on this today. I think for now we can get lock into 0.13.0 for now and I can follow up with a separate issue related to the FileType thing in 0.14.0.

For now Im going to switch gears to spike on seeing we can verify using jacoco work ( in sbt form ) to get both java and scala coverage. That would likely be the coverage direction I'd like to explore since we haven't been able to get scoverage to cover java sources.

@ittaiz
Copy link
Member

ittaiz commented Jun 19, 2018 via email

@johnynek
Copy link
Member

we use 14.1 without the --all_incompatible_changes flag at Stripe.

@softprops
Copy link
Contributor Author

that package name is unfortunate. Feel free to change it.

I'm going to do that in a separate pull to keep the diff for the mixed compilation thing tidy.

@softprops softprops mentioned this issue Jun 20, 2018
Merged
ittaiz added a commit that referenced this issue Jul 14, 2018
@ittaiz
add scala stdlib to java compile classpath, relates to #526
263284b
@ittaiz ittaiz mentioned this issue Jul 14, 2018
Merged
johnynek pushed a commit that referenced this issue Jul 16, 2018
@ittaiz @johnynek
add scala stdlib to java compile classpath, relates to #526 (#558)
329543d 
* add scala stdlib to java compile classpath, relates to #526

* linting
@johnynek
Copy link
Member

I think this is currently solved by #558 please reopen if that is false.

@softprops
Copy link
Contributor Author

Thanks for hopping on this folks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@softprops @johnynek @ittaiz