Disable checkImplicitDependencies by default on conjure subprojects and make existing dependencies explicit #1652
Conversation
Generate changelog in
|
| } | ||
|
|
||
| private static void configureDependencies(Project project, ConjureExtension extension) { | ||
| project.getDependencies().add("api", Dependencies.CONJURE_JAVA_LIB); |
There was a problem hiding this comment.
why do these now only get applied if Dependencies.isObjects is true?
There was a problem hiding this comment.
I believe they aren't actually used by the others, and decided it was better to be consistent with non-local codegen. I'll double-check with a local publication of the plugin as well (I think I already had, but will reverify since it's been a while)
There was a problem hiding this comment.
@bjlaub I've tested locally on a couple of internal repos (including using normal conjure as well as conjure-java-local), which showed that writing the locks and compiling everything was fine. I'd like to test with an RC as well, if everything else looks good to you
| static final String GUAVA = "com.google.guava:guava:33.4.8-jre"; | ||
| static final String UNDERTOW_CORE = "io.undertow:undertow-core:2.3.18.Final"; | ||
| static final String JACKSON_ANNOTATIONS = "com.fasterxml.jackson.core:jackson-annotations:2.18.3"; | ||
| static final String JACKSON_DATABIND = "com.fasterxml.jackson.core:jackson-databind:2.18.3"; | ||
| static final String FINDBUGS = "com.google.code.findbugs:jsr305:3.0.2"; | ||
| static final String ERROR_PRONE_ANNOTATIONS = "com.google.errorprone:error_prone_annotations:2.38.0"; | ||
| static final String SAFE_LOGGING_PRECONDITIONS = "com.palantir.safe-logging:preconditions:3.9.0"; | ||
| static final String SAFE_LOGGING = "com.palantir.safe-logging:safe-logging:3.9.0"; |
There was a problem hiding this comment.
To check: we may also need a dependency on conjure-java-runtime-api somewhere to depend on RemoteException? (for dialogue-generated clients)
[skip ci]
| // We disable the wrapper checkImplicitDependencies task in conjure subprojects, so users can configure the | ||
| // global check task to depend on it, even if the conjure generator version they use happens to | ||
| // implicitly depend on transitive libraries that the current gradle plugin version does not declare | ||
| // explicitly. | ||
| // We do still try to set up dependencies explicitly, and cover it in ConjurePluginTest, but it could | ||
| // otherwise create problems when versions of the generator and plugin are not in sync. | ||
| proj.getTasks().withType(checkImplicitDependenciesParentTask).configureEach(task -> { | ||
| task.setEnabled(false); | ||
| }); |
There was a problem hiding this comment.
This seems to not work exactly the way I intended it too, as I see e.g. the checkImplicitDependenciesMain tasks still be run. I'll have to dig deeper into this, but likely won't have time for now. I'll get back to this once I'm back from holidays
|
This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days. |
Before this PR
In the current state of things, the gradle-conjure plugin is not properly setting up dependencies for the conjure subprojects.
Specifically, it does not pass the
checkImplicitDependenciestask defined by BaselineExactDependencies.This means that if users want to wire up e.g.
tasks.check.dependsOn checkImplicitDependencies, they also have to manually disable it within conjure subprojects.We should define the dependencies explicitly and have this task pass. However, due to the way this plugin works, we cannot guarantee this will work.
This is due to the fact that the conjure-java generator may upgrade ahead of the gradle-conjure plugin itself, leading to the following case:
gradle-conjurecurrently defines the exact dependencies appropriately and a consumer repository wired up thecheckImplicitDependenciestask to run oncheckconjure-javacode generation adds a dependency on a new type, which happens to come from an existing transitive dependencygradle-conjureplugin isn't updated yet, butconjure-javagets bumped in a consumer repositorycheckImplicitDependenciestask then starts failing, because of the new transitive dependency.conjure-javaupgrades untilgradle-conjureis fixed and upgraded as well, or until someone comes and manually disables the check taskcheckUnusedDependenciesmay require you to upgrade both in lockstep, since the new dependency is unused in previous generator versionsInstead, it's better to simply disable the
checkImplicitDependenciestask on conjure subprojects.To ensure we do still have explicit dependencies as best as we can, we should also have tests that verify the dependencies are explicit.
After this PR
==COMMIT_MSG==
Disable checkImplicitDependencies by default on conjure subprojects and make existing dependencies explicit
==COMMIT_MSG==
Possible downsides?