Adds a Bloop-specific Gradle project configuration #106
Conversation
Some Gradle experts at $WORK have told me that the way we were previously resolving artifacts wasn't strictly correct due to two reason: - Project references in project configurations (like the ones we resolve in `getConfigurationArtifacts`) are allowed to be empty for now, but that will change, and it's a bug. gradle/gradle#26155 - The way we currently resolve artifacts doesn't let Gradle know our intention (as we can resolve any arbitrary configuration) and it doesn't play well with Gradle transforms, which require all of the resolved configuration artifacts to be present, otherwise it fails with a `FileNotFoundException`. As a result, I changed the code to introduce a Gradle configuration `bloopConfig` that extends all those configurations we care about, and change the resolution code to only depend on that configuration. It seems that solution works and passes all tests, and it avoids the `FileNotFoundException` error I found out. Because we now have a proper `configuration`, it means that users that create their own configuration and want it to be exported to bloop need to do `bloopConfig.extendsFrom(myConfig)` where `myConfig` is a user-defined configuration. The reason for this is because when we declare that our configuration extends all the other existing configuration, we do it at evaluation time. User-defined configurations run after the bloop plugin, so they never get added unless the user wishes to. This is not a big deal as custom configuration are extremely rare, and this is a common practice in Gradle land when you define your own configuration.
| @@ -3632,6 +3632,7 @@ abstract class ConfigGenerationSuite extends BaseConfigSuite { | |||
| | | |||
| |configurations { | |||
| | scalaCompilerPlugin | |||
| | bloopConfig.extendsFrom(scalaCompilerPlugin) | |||
There was a problem hiding this comment.
It actually is, because as I say in the description this configuration is user-defined. If it was scalaCompilerPlugins it would be extended on by default. But this one is user-defined, so users need to extend from it if they want the artifacts to be exported to the resolution module.
The great thing about this approach is that it allows users to configure which artifacts are exported, which before wasn't possible.
There was a problem hiding this comment.
OK. But if I run this test with or without that extendsFrom line, semanticdb-scalac_2.12.8 is always present in the resolution/modules. Is that what the difference is supposed to be? I'm using Gradle 8.4 and jdk 21.
There was a problem hiding this comment.
I have no idea why, but this test was failing for me. This was the way to make it work.
It might or might not be needed, I'm just doing sbt +test to check everything works.
Does that work for you without this extendsFrom?
There was a problem hiding this comment.
Yes - without that line the test still passes for me - I can't get it to fail no matter what Gradle and JDK version I use.
It would make sense to me that the line isn't needed as in BloopPlugin#extendCompatibleConfigurationAfterEvaluate you extend every project.getConfigurations. Wouldn't that include the user-defined ones? And before this PR the code just iterated through project.getConfigurations. I think most projects would want all configs included by default wouldn't they?
| .filter(_.isCanBeResolved) | ||
| .flatMap(getConfigurationArtifacts) | ||
|
|
||
| // val allArtifacts2 = project.getConfigurations.asScala |
There was a problem hiding this comment.
Will remove this (as well as other commented out code) before the final merge!
The previous solution worked, but it was not ideal because it introduced
the concept of a `bloopConfig` that had to extend all the configurations
in the project (even across different source sets, e.g. main, test,
integTest, etc.).
This is awkward because the plugin generates a bloop config per source
set. So, if we have one bloop config extending all configurations from
all source sets, and then depending on that when generating bloop config
per source set, we're running into a lot of repeated resolutions to
produce the necessary artifacts and the resolution config section
contains a lot more artifacts than it should (e.g. foo.json and
foo-test.json would contain the same set of artifacts). Also, in theory,
this approach could also cause Gradle to add the wrong artifact version
because it would need reconciling all artifact across all source sets
is more likely to produce different results. This is a theoretical
point, but it'd be nice to avoid it.
So, the fix to all of this is to make a `bloopConfig` configuration per
source set. For example, the main source set gets a `bloopConfigMain`
configuration that extends from some default configurations that we care
about for Java/Scala plugins.
(In the Android world, there are no sourcesets, but "variants" so we
fallback on the previous behavior and depend on all the sources for
`bloopConfigAndroid`, a config that exists only in Android projects and
that adds the artifacts of all the project variants -- this is the
behavior one expects, and in this case we extend from all the
configurations in the variants for backwards compatibility.)
To make the process more customizable for end users, I've also added a
new property in the bloop configuration that lets users specify the
extra configurations that should be extended per source set:
```
bloop {
extendUserConfigurations = [configurations.myCustomConfig.name]
}
```
This way, users have control over the configurations that should also
contribute artifacts to the resolution section in the bloop config.
This is nice and better than the previous approach.
|
@Arthurm1 what do you think? Is this good to merge? My Gradle knowledge is a bit limited these days |
| sourceSet.getRuntimeOnlyConfigurationName(), | ||
| sourceSet.getRuntimeClasspathConfigurationName(), | ||
| sourceSet.getRuntimeElementsConfigurationName(), | ||
| "scalaCompilerPlugins" |
There was a problem hiding this comment.
I think only getCompileClasspathConfigurationName(), getRuntimeClasspathConfigurationName() and scalaCompilerPlugins is needed here as the others aren't resolvable. See tables
|
@tgodzik Makes sense and, except for my last comment just now, it looks good to me. Could be merged as is, I just think the other I guess it's possible that artifacts would not be exported where they were before but I can't think of a case where that would be an issue. |
|
Thanks for confirming! @jvican would you mind removing them or is it something you found actually necessary? |
|
Hey folks - let me follow up on this soon. The gradle experts at work have raised more concern around some bloop internals that are violating some soon-to-be-deprecated behavior in Gradle 9, and it seems we need a bit of a broader discussion aside from this PR. I'd like to get this merged at some point, but let's first get that discussion with the Gradle experts going and seek their advice. I'll open a bloop ticket and redirect them there so the discussion is open. |
Some Gradle experts at $WORK have told me that the way we were
previously resolving artifacts wasn't strictly correct due to two reasons:
Project references in project configurations (like the ones we resolve
in
getConfigurationArtifacts) are allowed to be empty for now, butthat will change, and it's a bug. Configuration resolution should fail if project dependencies have not yet been built gradle/gradle#26155
The way we currently resolve artifacts doesn't let Gradle know our
intention (as we can resolve any arbitrary configuration) and it
doesn't play well with Gradle transforms, which require all of the
resolved configuration artifacts to be present, otherwise it fails
with a
FileNotFoundException.As a result, I changed the code to introduce a Gradle configuration
bloopConfigthat extends all those configurations we care about, andchange the resolution code to only depend on that configuration.
It seems that solution works and passes all tests, and it avoids the
FileNotFoundExceptionerror I found out.Because we now have a proper
configuration, it means that users thatcreate their own configuration and want it to be exported to bloop need
to do
bloopConfig.extendsFrom(myConfig)wheremyConfigis auser-defined configuration. The reason for this is because when we
declare that our configuration extends all the other existing
configuration, we do it at evaluation time. User-defined configurations
run after the bloop plugin, so they never get added unless the user
wishes to. This is not a big deal as custom configuration are extremely
rare, and this is a common practice in Gradle land when you define your
own configuration.