search

higherkindness / rules_scala

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

Cannot configure scala_deps_used via --define #249

Closed joprice closed 4 years ago

joprice commented 4 years ago

I tried configuring scala_deps_used in my .bazelrc as the docs suggest with the line:

build --define=scala_deps_used=warn

But I could not get it to respect the setting. I grepped the repo and found that configure_zinc_scala has a deps_used parameter, and using this worked as expected. Are the docs out of date, or is this define only used for the default scala version and ignored when defining a separate one with configure_zinc_scala?

borkaehw commented 4 years ago

You might be right. I don't think we take --define into account when we do deps check. Do you think it's a nice to have feature in the .bazelrc file? Or configuring it in configure_zinc_scala is good enough?

jjudd commented 4 years ago

Maybe I'm wrong, but isn't --define how the deps check feature works? Like you do --define=scala_deps_used=warn or --define=scala_deps_used=error?

joprice commented 4 years ago

I don’t know enough to know whether it’s broken or I’m using it wrong. For now having it in configure_zinc_scala works since I configure it statically, but it being able to be dynamic without having to define a custom config setting would be convenient for different strictness levels when refactoring or in CI envs.

borkaehw commented 4 years ago

@jjudd I can't see where we use --define anywhere. As far as I understand, --define is used to configure attributes in BUILD, see reference here. The most straightforward way is to pass in error, check, or off via attributes in configure_zinc_scala.

@joprice I will try if I can come up with something handy like that. Before that, I would check out select to see if it fits your need.

joprice commented 4 years ago

@borkaehw I was mostly concerned about the docs mentioning this facility, but it not being possible. It took me a while to find that it was available on configure_zinc_scala and wanted to save other people time. I don't currently need the feature to configure it via a define, just anyway to set it.

borkaehw commented 4 years ago

@joprice Sorry for the inconvenience. I talked to the other maintainers, it used to work. We will either remove it from doc or actually fix it. Please keep this issue open as a reminder. I will keep you posted.

joprice commented 4 years ago

I just hit this again and had to copy the default 2.12 toolchain into my project to override deps_used. At that point, it would be more convenient to just have a flag, since now I have to maintain a bit more just for that one flag, and it wasn't obvious that I was supposed to do that. On the other hand, maybe it could also be solved by allowing passing in extra config to scala_register_toolchains or something along those lines.

joprice commented 4 years ago

@borkaehw I tried the changes and get a duplicate key error when trying to configure them via the zinc toolchain macro. Using define works now, but I expected to be able to set it to "warn" as well as off. Only "off" seems to work.

borkaehw commented 4 years ago

Help me understand it correctly. There are still two issues

  1. Using --define in .bazelrc and Zinc toolchain macro causes duplicate key?
  2. --define doesn't support warn, but Zinc toolchain does?