-
Notifications
You must be signed in to change notification settings - Fork 237
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.
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
Consider an optional switch to turn all deps
into exports
, allowing access to all transitive classes just by depending on the top level artifact
#147
Comments
As an opt-in feature, this is great. There are plenty of libraries that people depend on via Maven that are designed to bring in others transitively (for example, Selenium's Ideally, this could be configured at the repository level (if I'm pulling from Maven Central, I want the default maven behaviour, but if I'm pulling from a local maven repo maybe something more disciplined makes sense) At the artifact level would end up being super-noisy in the common case. |
deps
into exports
, allowing access to all transitive classes just be depending on the top level artifactdeps
into exports
, allowing access to all transitive classes just by depending on the top level artifact
I think this is required for scala support: the way the types work means that all libraries need to be compiled with symbols from transitive dependencies available during compilation. |
Should we have this as an opt-in feature per artifact? That is, maven_install(
artifacts = [
maven.artifact("group.id", "artifact", "1.2.3", export_transitive_compile_deps = True),
],
) instead of applying this across the entire graph. So, for the following example, rules_jvm_external/regression_testing_install.json Lines 63 to 84 in 9e8ab91
we would place rules_jvm_external/regression_testing_install.json Lines 65 to 68 in 9e8ab91
into |
@shs96c what do you think? I think this is a more controllable approach than a |
No matter what happens, the global setting will be needed anyway to define the default approach for whether or not dependencies are exported. |
I actually had the opposite reaction, and thought Jin's idea was a great way to enable the use case while containing the blast radius. Seems easier to start contained and then go global if that's still needed, than to go the other way. |
Making it at a global level will most likely to cause most targets to depend on a much, much larger compile time classpath than they actually require. e.g. rules_jvm_external/regression_testing_install.json Lines 5262 to 5282 in 027db97
brings in 19 other jars. With this, I think the default behavior should be opt-in at the local artifact level to help keep track of classpath bloat per top level artifact, instead of bloating up the entire dependency tree for artifacts that don't necessarily need it. |
I think there is a use case for both, especially depending on whether you use a single
In our case we have a mix of both: we have a single |
I'm happy to take a shot at this over the weekend, unless anyone else is already on it, @shs96c? |
I have a PR ready to go, but I'd like to get some folks to look at it before I can submit it. If you beat me to it, please go for it! |
While I get that's what maven and gradle users are used to, it's one of the worst decisions ever made in the maven/gradle space. It's the opposite of "Strict deps". It has led to so much bad graph specification in my experience of several decades of the maven ecosystem. Indeed, we're doing the exact opposite in our gradle builds, figuring out lint rules or gradle build code to constrain projects so you must declare all the deps you use, and not "get it for free" from the transitive closure. I cannot disagree more with this recommendation in practice. This is one of those decisions that feels convenient, but has the effect of concealing problems in the deps graph, because you aren't specifying what you use. Maven has always supported this transitive convenience, but it has also recommended against the practice, from the get-go. From the aforelinked doc: "Although transitive dependencies can implicitly include desired dependencies, it is a good practice to explicitly specify the dependencies you are directly using in your own source code. This best practice proves its value especially when the dependencies of your project changes their dependencies." That said, we can't police users and prevent them from every choice one might make differently. If users will forgo the benefits of your tool because they insist on this convenience, then I'd rather they use the tool. But still, I would advise that it be recommended against in the strongest terms. |
It's also a bit horrifying to me that scala actually requires this, but I can also see the case for enabling it for scala users' use. But with the appropriate caveats. |
@cgruber, I totally agree with everything you said. This above was really the crux of it for me, and I've always been on the same page. Nonetheless, if you want to get people to move in a positive direction, it's something that unfortunately falls on the less popular tools to provide a pleasant ramp, otherwise it's just another massive barrier to entry. It's incredibly frustrating to be faced with a preexisting mess that you wisely avoided, and still have it be your problem. The one thing I guess this makes me think is: are we thinking too small when we talk about making the build tool optionally accept the pernicious behavior? I know I have certainly been thinking this way. Would it be better if there was a tool that helped isolate the deps, something git-bisect-like or some other way to do it? I don't know if that's possible, but it's made me want to think about it a bit. |
That's a separate matter, but, yes, bazel should be helping people far more. The IJ plugin should be doing things like suggesting new dependencies, and highlighting unused dependencies. The behaviour of |
I ran into this just yesterday (cf. Issue #275). One note I would like to make, is that prominent projects actually perpetuate the issue by instructing users to implicitly rely on the transitive nature of maven. For example: SLF4J instruct in their user manual to just depend on And scala-logging gives the same instructions. Probably by following the advise from SLF4J's user manual. I have no strong opinion on implementing this on a Most importantly, I would like this to be documented and highlighted well enough. Perhaps in a distinct section on differences to Maven. |
With #285, auto-suggestions for
This is also integrated into the IntelliJ plugin as a clickable action: I'm leaning towards closing this issue and not allowing transitive exports in favor of buildozer suggestions like the above. This keeps the build graph minimal and isn't tedious to maintain. |
While that integration is very helpful, it doesn't solve this issue, as it is less about the amount of work needed to declare all the transitive dependencies, but more about the fact that the transitive dependencies are not something one should depend on in the first place: in both the scenarios I described above, if changes in the libraries' versions result in changes to the transitive dependencies one should not need to update all their |
Does it break expectations from library authors when transitive dependencies aren't exposed in the compile classpath for compile-scoped Maven publications? For example, if a library author publishes a library with Gradle, which provides this guidance:
then it would be surprising for the library author to find that bazel doesn't honor this. As a more concrete example (published with sbt instead of gradle, but still under POM semantics), scalapb-runtime 0.9.4 has a compile-scoped dependency on com.thesamet.scalapb:lenses because the scalapb-runtime class FileDescriptorProto extends scalapb.lenses.Updatable. Without the lenses library in the compile classpath, bazel fails with:
|
Is there any update on this? One thought would be to auto-emit a synthetic target that does re-export all the |
Referring to Uri's point, I'm not sure I follow. |
@shs96c It's not that the classpath doesn't contain the transitive dependencies, it's that if the compiler loads them |
This was motivated by a conversation with @simonstewart for better developer ergonomics and gradual adoption, because this is what Maven and Gradle users are used to.
The general idea is to depend on just the top level dependency, and get all the classes in the transitive classpath for free. This was previously implemented by @davidsantiago in jin/rules_maven#9, but we removed it in jin/rules_maven#16 with the concerns stated in the first comment and @dkelmer's comment in jin/rules_maven#16 (review)
We should consider the tradeoffs between ergonomics and the growth of classpath complexity. This can be an opt-in feature with major disclaimers on its implications.
The text was updated successfully, but these errors were encountered: