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

Add option to pass lib dependencies to coursier #2628

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

987Nabil
Copy link
Contributor

@987Nabil 987Nabil commented Jun 8, 2022

This can be used to add custom url handlers to coursier, so
scala-steward could for example reach s3 buckets, google cloud storage
or artifactregistry.
See https://get-coursier.io/docs/extra.html#extra-protocols and
coursier/coursier#1987

@987Nabil
Copy link
Contributor Author

987Nabil commented Jun 8, 2022

@fthomas or @exoego I got the idea for this based on coursier/coursier#1987 and I think it makes sense to have this in steward too.
I guess as a future improvement, we could read the config value csrProtocolHandlerDependencies from sbt when importing sbt projects.

Also I am a bit stuck with a test strategy, maybe you can give me an idea or hint? So far I did build steward local and tested with a locally published version of a self written lib to handle artifactregistry:// urls. That worked well.

@987Nabil 987Nabil force-pushed the coursier-custom-dependencies branch from 6bf7454 to 6203ee3 Compare June 8, 2022 22:10
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 81.41%. Comparing base (7ec418c) to head (521f3d0).
Report is 3 commits behind head on main.

❗ Current head 521f3d0 differs from pull request most recent head 34318b6. Consider uploading reports for the commit 34318b6 to get more accurate results

Files Patch % Lines
...a/org/scalasteward/core/coursier/CoursierAlg.scala 90.47% 2 Missing ⚠️
...d/core/coursier/CoursierDependenciesFetchAlg.scala 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2628      +/-   ##
==========================================
- Coverage   91.18%   81.41%   -9.78%     
==========================================
  Files         166      147      -19     
  Lines        3404     2615     -789     
  Branches      317       42     -275     
==========================================
- Hits         3104     2129     -975     
- Misses        300      486     +186     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@exoego exoego added the enhancement New feature or request label Jun 23, 2022
@987Nabil
Copy link
Contributor Author

987Nabil commented Jul 18, 2022

fyi: I created this lib, that can be added this way and I am using a build of the branch of this PR to run steward daily for a bunch of repos. Works just fine.

@fthomas
Copy link
Member

fthomas commented Jul 19, 2022

fyi: I created this lib, that can be added this way and I am using a build of the branch of this PR to run steward daily for a bunch of repos. Works just fine.

Nice! I'm wondering: would this also work without this patch if gar-coursier would just be added to this project's libraryDependencies?

@987Nabil
Copy link
Contributor Author

I think I tried that at some point and it did not. But I'll check again and let you know.

@987Nabil
Copy link
Contributor Author

@fthomas I just realized, that it can't work as a dependency of the project, since we already load a plugin from GAR. But even if I would add it to /project, it can't work. Coursier needs the dependency to be loaded in the class loader before resolving the service specific url. For example artifactregistry://.... And coursier does not load downloaded libs into the class loader. And even if it would, how could it know that it has to load gar-coursier before the dependency that needs the URL resolver it contains?
The only way I see is, that we tell coursier to load this dependency first.
fyi: For resolving it during project load, we use sbt-gcs-resolver. But its code is only working with sbt.
I guess one could argue, to include the capability of resolving URLs from google, amazon and other famous services directly in scala steward. But I kinda fear, that this would either blow over time or is not flexible enough for some edge solutions.

@fthomas
Copy link
Member

fthomas commented Aug 14, 2022

I experimented a little bit with adding gar-coursier as a dependency to the project. According to https://get-coursier.io/docs/extra.html#extra-protocols, Coursier should search for that plugin via the coursier.cache.CacheUrl.getClass.getClassLoader classloader. So I put the following code into Scala Steward's main

        println(
          _root_.coursier.cache.CacheUrl.getClass.getClassLoader
            .loadClass("coursier.cache.protocol.ArtifactregistryHandler")
        )

and observed what is printed with and without gar-coursier as a dependency. Without the additional dependency, loadClass throws a ClassNotFoundException as one would expect:

[error] Exception in thread "main" java.lang.ClassNotFoundException: coursier.cache.protocol.ArtifactregistryHandler
[error] 	at java.net.URLClassLoader.findClass(URLClassLoader.java:387)
[error] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
[error] 	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
[error] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
[error] 	at org.scalasteward.core.Main$.run(Main.scala:29)
[error] 	at cats.effect.IOApp.main(IOApp.scala:268)
[error] 	at cats.effect.IOApp.main$(IOApp.scala:203)
[error] 	at org.scalasteward.core.Main$.main(Main.scala:23)
[error] 	at org.scalasteward.core.Main.main(Main.scala)

If I add "io.github.987nabil" %% "gar-coursier" % "1.1" to the libraryDependencies of Scala Steward's core module, I get following output:

[info] class coursier.cache.protocol.ArtifactregistryHandler

The same happens with Thread.currentThread().getContextClassLoader.

This suggests to me that it should be possible to use gar-coursier with Scala Steward without any modifications by just adding the gar-coursier JAR to Scala Steward's classpath.

@987Nabil
Copy link
Contributor Author

987Nabil commented Sep 1, 2022

Hey @fthomas sorry, for the late answer it got buried in my notifications. I will try to make it run the in the way you suggested. If I am successful, I'll only make a PR to add documentation on how to use custom protocol handlers with Scala Steward.

@sideeffffect
Copy link
Contributor

sideeffffect commented Jan 26, 2023

@987Nabil thanks for working on this! Have you had some luck with the experiment?
How can one easily add libraries to Scala Steward's classpath?

@fthomas
Copy link
Member

fthomas commented Feb 15, 2023

How can one easily add libraries to Scala Steward's classpath?

That depends how you run Scala Steward, right? For the GitHub Action it could be possible to use Coursier's --extra-jars command-line option to pass additional JARs to Scala Steward's classpath, see: scala-steward-org/scala-steward-action#466.

@fthomas
Copy link
Member

fthomas commented Feb 17, 2023

For the GitHub Action it could be possible to use Coursier's --extra-jars command-line option to pass additional JARs to Scala Steward's classpath, see: scala-steward-org/scala-steward-action#466.

Btw, it would be helpful if anyone who uses an extra protocol handler like gar-coursier and the Scala Steward GitHub Action to try and test the extra-jars input of the action. It would be nice to get confirmation that just adding JARs to the classpath is enough to enable support for additional protocols. Negative results would be helpful too. :-)

@fthomas
Copy link
Member

fthomas commented Mar 20, 2023

Linking sbt/sbt#6375 here. I guess if that PR is merged eventually, it would be nice if Scala Steward takes csrProtocolHandlerDependencies into account automatically.

This can be used to add custom url handlers to coursier, so
scala-steward could for example reach s3 buckets, google cloud storage
or artifactregistry.
See https://get-coursier.io/docs/extra.html#extra-protocols and
coursier/coursier#1987
@987Nabil 987Nabil force-pushed the coursier-custom-dependencies branch from 521f3d0 to 437388f Compare February 29, 2024 16:20
@rolang
Copy link
Contributor

rolang commented Mar 2, 2024

Linking sbt/sbt#6375 here. I guess if that PR is merged eventually, it would be nice if Scala Steward takes csrProtocolHandlerDependencies into account automatically.

Yeah, it doesn't seem to take it into account yet, we added an extra protocol handler in sbt via csrConfiguration like

csrConfiguration := csrConfiguration.value.withProtocolHandlerDependencies(
  Seq("org.example" % "google-artifact-repository-coursier" % "0.1.0")
)

but it didn't help 🤷‍♂️ . We also got exceptions from the sbt-plugin as described in this PR
scala-steward-org/sbt-plugin#48

For now I just added an extra protocol handler as core dependency together with the sbt-plugin update like in this commit to get it working .

Would be good if scala-steward could pick up the coursier configs from the sbt project or accept an extra protocol handler as parameter.

@mzuehlke
Copy link
Member

mzuehlke commented Mar 4, 2024

If I read this right, the https://github.com/scala-steward-org/sbt-plugin would need additional logic to export the additionally protocol handler from the csrConfiguration key.
This can the be picked up by Scala Steward and passed to coursier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants