higherkindness / rules_scala

Robust and featureful Bazel rules for Scala
Apache License 2.0
66 stars 28 forks source link

deps_used error when used with rules_docker's scala_image #255

Closed joprice closed 4 years ago

joprice commented 4 years ago

I am getting a remove deps buildozer error when using these rules with rules_docker via emulate_rules_scala. Those rules don't have the concept of deps_used_whitelist. I tried using both deps and runtime_deps, but couldn't silence the error. This is unfortunate, since without a workaround, users in this situation will be encouraged to disable this useful feature.

borkaehw commented 4 years ago

Could you provide more information how did you set up your build files? Do you mean it's trying to remove a dependency that is not in deps?

joprice commented 4 years ago

Sorry, I wasn't very clear. It's trying to remove the single dependency that is a scala_library which provides the main class. In other calls to scala_binary, I have to set deps_used_whitelist to avoid this issue as well.

borkaehw commented 4 years ago

that is a scala_library which provides the main class

It doesn't sound like the right way.

It would be easier for us to help if you can provide minimal reproducible case.

joprice commented 4 years ago

Here is a repo that reproduces the issue https://github.com/joprice/bazel_experiments

borkaehw commented 4 years ago

I am not familiar with how scala_image works. If you are not explicitly using the targets in deps, the buildozer command will show up. It seems like we don't have tests for rules_docker compatibility. We are happy to take PR if you have idea how to fix it.

@SrodriguezO Do you have insight on it?

SrodriguezO commented 4 years ago

scala_image expects bazelbuild/rules_scala rules, but it should still work with higherkindness when you emulate the bazelbuild ones (as @joprice did in his example).

There were two issues though:

  1. I think you're using scala_image incorrectly--it shouldn't depend on a scala_binary; rather, it should specify which sources it's using directly (as shown in the rules_docker docs).
  2. There was a bug in our emulation logic where if None was specified for deps (which seems to be scala_image's default when no deps are specified), things broke. Here's a fix: https://github.com/higherkindness/rules_scala/pull/257

Another option is to use rules_docker's container_image instead of scala_image. You'll have to look at the docs to see how that one works (it should work without emulating bazelbuild/rules_scala)

Sidenote: the reason the buildozer command won't work is that the targets you're dealing with are generated by rules_docker

joprice commented 4 years ago

I see. I'm using legacy code that I want to build with minimal modifications, but I could move the main class out into it's own file. I was also trying to make use of a scala_binary rule that existed to avoid repeating the srcs. So the idea is that you have a separate file for your main, and the deps includes whatever other sources defined as scala_library that has the rest of the files?

SrodriguezO commented 4 years ago

I'm using legacy code that I want to build with minimal modifications

Does your current code already use scala_binary and scala_image?

So the idea is that you have a separate file for your main, and the deps includes whatever other sources defined as scala_library that has the rest of the files?

It seems rules_docker expects you to keep all your library targets as they already are and to then "simply rewrite scala_binary to scala_image" (from their docs); so if all your sources are currently in the scala_binary, you should be able to simply keep them in the scala_image

joprice commented 4 years ago

I assumed it was better for incremental compilation, but maybe zinc handles that. Also, it felt natural to have a binary, image, and tests depend on a library that is the one place specifying the glob of srcs.

SrodriguezO commented 4 years ago

The less you rely on zinc the better. I do think breaking up your build with scala_library would be a good idea, but this is independent of getting scala_image to work. I've personally never used scala_image, but it seems it's meant to simply replace the scala_binary in your design

joprice commented 4 years ago

I assume adding the file that defines the main class to the scala binary and the scala image will result in that single file being compiled twice, which isn't that big of a deal if it's just a dedicated main class whose implementation relies mostly on other files. I'll try that first as a temporary workaround and then experiment with a custom scala_image rules that uses these rules without emulating the other ones.

SrodriguezO commented 4 years ago

I assume adding the file that defines the main class to the scala binary and the scala image will result in that single file being compiled twice

Do you mean you're using both a scala_binary and a scala_image? You should only be using scala_image--it replaces scala_binary

joprice commented 4 years ago

I'm defining a library, a binary, and an image, where the image uses the binary and the binary uses the image (library <- binary <- image). The binary is used locally and for testing, while the image is for distribution. Reusing the binary was convenient to avoid having to repeat arguments like data, jvm_flags, resources, etc, and to avoid getting them out of sync. If that's the wrong approach, I can create local variables for those and reference them in the binary and image.

SrodriguezO commented 4 years ago

I see. I've never used scala_image, but based on their docs I don't believe it should be using a scala_binary.

The binary is used locally and for testing, while the image is for distribution. Reusing the binary was convenient to avoid having to repeat arguments like data, jvm_flags, resources, etc, and to avoid getting them out of sync. If that's the wrong approach, I can create local variables for those and reference them in the binary and image.

An alternative might be to create a macro that generates both a scala_image and a scala_binary with the same attributes? Might be a bit cleaner than using variables, and you could reuse it for other targets :)

joprice commented 4 years ago

Yea, that's probably what I'll end up with.

SrodriguezO commented 4 years ago

It seems scala_image already generates a binary for you! I can't find quick confirmation on their docs (you may have to dig more), but for a scala_image target named image, I can build :image.binary:

INFO: Analyzed target //:image.binary (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:image.binary up-to-date:
  bazel-bin/image.binary-bin
  bazel-bin/image.binary.jar
INFO: Elapsed time: 0.259s, Critical Path: 0.03s
INFO: 0 processes.
INFO: Build completed successfully, 3 total actions
SrodriguezO commented 4 years ago

This is the target the buildozer command wanted to update on your example: buildozer 'remove deps //:service' //:image.binary

Since it's generated by scala_image, however, buildozer couldn't find it.

SrodriguezO commented 4 years ago

@joprice do you mind if I close this issue? Or is there anything else we need to address here?

joprice commented 4 years ago

I think I understand what's going on. I'll try it out and leave a comment if I'm still stuck. Thanks for all the help!