Skip to content
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

Make KSP dependency optional to avoid Google Maven Repo #2779

Closed
A248 opened this issue Jul 24, 2021 · 6 comments · Fixed by #2848
Closed

Make KSP dependency optional to avoid Google Maven Repo #2779

A248 opened this issue Jul 24, 2021 · 6 comments · Fixed by #2848

Comments

@A248
Copy link

A248 commented Jul 24, 2021

I understand that dagger 2.38 now requires adding the Google maven repository. I'm wondering if there's a way to avoid the requirement that users add this repository to their build files.

Adding third-party maven repositories to the build is often discouraged, especially for artifacts which themselves publish to central. As noted:

At present, this won't preclude your project from being included, but we do strongly encourage making sure all your dependencies are included in the Central Repository
https://maven.apache.org/repository/guide-central-repository-upload.html

Based on what I read in #2349 , KSP is used for processing annotations in Kotlin source code. For Java-based applications, I would assume KSP isn't needed. At the very least, I'd expect that the dependency on KSP could be made optional with some slight refactoring.

Is it possible to use dagger-compiler without KSP so users don't need the Google Maven Repo?

@Chang-Eric
Copy link
Member

Thanks for bringing this up, this is something we discussed at length internally before making that change. I think the answer is we're kind of stuck, but let me try to explain the details behind that.

So the issue is that KSP changes the basic types the Dagger processor uses, like every time we need to look something up on one of the classes we're processing, that base representation we use has to change from a Java type to a KSP type. You can imagine that this is everywhere in our code and as mentioned in that issue, even leaks through our SPI plugin. This makes it sadly not really possible to factor out of the codebase without simply forking the whole thing, which isn't really maintainable for us.

With that, if we accept that we cannot split the dependency out, then our other option is to get KSP to release to Central. While this may be possible with some help from the KSP team, we also have another dependency that only lives on Google Maven coming up which is a dependency on XProcessing (via Room). This is a library that helps manage the fact that even when you have dependencies on both Java annotation processing APIs and KSP, you still have to duplicate a lot of code inline with what you can imagine would be if-statements that call basically the same methods.

I don't know of any technical problems with getting both of these dependencies on Central, but it would take some time between coordination with other teams and actually getting it done (even after assuming they agree to do it), and the issue is that if we wanted to block on this it would block us from starting to do any migration at all. KSP is a high priority for us due to the expected impact on build times, so we didn't want to delay starting this given that it is already going to be a long project. So we felt that requiring the Google Maven dependency was the least bad option, even though we know it is still not a great thing to do.

@A248
Copy link
Author

A248 commented Jul 28, 2021

I appreciate the forthright and comprehensive explanation. It's understandable that Dagger cannot avoid the dependency on KSP. My objective, as you seem to understand, is to avoid requiring the Google Maven Repo.

The issue isn't that I don't trust Google to serve artifacts, but rather the concern for maintaining best practices. It is a well-established tenet of the Maven-driven ecosystem that OSS libraries be located on Maven Central. Another one of the costs of third-party repositories is, in Maven at least, they leak into dependents' builds. Your users, including myself, would be grateful if you could limit dependencies to Central.

Long-term, having KSP and XProcessing publish to Maven Central seems the most ideal solution. As you point out, it will require some time. For such a process, it would be wise to start the wheels turning early. I imagine publication to Central would also benefit other consumers of KSP and XProcessing besides dagger. Would it be appropriate to open a relevant issue on the ksp tracker?

In terms of a more temporary solution, have you considered shading KSP and whatever other artifacts you need from Central into dagger-compiler? If you shade and relocate these libraries, you can then publish a jar which doesn't declare dependencies on them. I don't see many disadvantages to shading your dependents. For sure, your methods will have different signatures after relocation is applied, but the fix for this hurdle is not difficult to implement. It is possible to publish two artifacts, one with shaded dependencies and the other with declared dependencies. Other libraries, including ByteBuddy, have done similar. Users who rely on dagger-compiler with shaded dependencies will not need the Google Maven Repo.

@Chang-Eric
Copy link
Member

Thanks, I created an issue on the KSP side for this (see the mention above).

I'm hesitant to publish two artifacts right now though, especially if one is to expose shaded classes as API. I'd like to see where the discussion with the KSP team goes first.

@Chang-Eric
Copy link
Member

Closing this now as it looks like they will start publishing to Central with beta07.

@dlazerka
Copy link

dlazerka commented Sep 8, 2021

Although this issue is closed, I believe it's not resolved yet. KSP started publishing to Central, right, but Dagger (as of version 2.38.1) still requires Google Android repository.

copybara-service bot pushed a commit that referenced this issue Sep 8, 2021
Fixes #2779

RELNOTES=N/A
PiperOrigin-RevId: 395460070
@bcorso
Copy link

bcorso commented Sep 8, 2021

@dlazerka thanks, I'll update the dependency to use a version on maven central.

@bcorso bcorso reopened this Sep 8, 2021
copybara-service bot pushed a commit that referenced this issue Sep 8, 2021
Fixes #2779

RELNOTES=N/A
PiperOrigin-RevId: 395460070
copybara-service bot pushed a commit that referenced this issue Sep 8, 2021
Fixes #2779

RELNOTES=N/A
PiperOrigin-RevId: 395460070
copybara-service bot pushed a commit that referenced this issue Sep 8, 2021
This change allows users to get the dependency from maven central rather than google's repository. This CL also removes the google repo from our (non-android) dagger tests/examples to validate that this repository is no longer needed. The android tests/examples still require this repository for AGP.

Fixes #2779

RELNOTES=Fixes #2779: Updates ksp dependency to latest version on maven central.
PiperOrigin-RevId: 395460070
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants