Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change scalafixResolvers to come from Coursier defaults so can be overridden #159

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

scottrice10
Copy link
Contributor

@scottrice10 scottrice10 commented Aug 25, 2020

The impetus for this proposed change is trying to use privately hosted scalafix rules with scala-steward. Currently, the scalafix tutorial describes how to add custom scalafix resolvers in a project's build.sbt. Since scala-steward adds the sbt-scalafix plugin on the fly, however, it would further need to edit a project's build.sbt in order to add custom resolvers, which would be tricky.

The alternative I'm proposing here sets the scalafixResolvers to Coursier's defaultRepositories. The defaults are the same as previously set - Repository.ivy2Local() and Repository.central(). But using defaultRepositories allows overriding the repositories via the COURSIER_REPOSITORIES environment variable, as documented here and shown in the defaultRepositories code.

I've tested this code locally with first doing

export COURSIER_REPOSITORIES="ivy2Local|central|ivy:https://a.b.c/d/f/[organisation]/[module]/(scala_[scalaVersion]/)(sbt_[sbtVersion]/)[revision]/[type]s/[artifact](-[classifier]).[ext]"

and then running sbt-scalafix on a repo (without overriding the scalafixResolvers), and verified it picks up the scalafix rules which exist only in the private ivy artifactory resolver.

When trying to run scala-steward to apply the privately hosted scalafix rules, the current error I'm getting is:

13:21:47  [error] (root/*:scalafixAll) scalafix.sbt.InvalidArgument: Failed to fetch [Dependency(Module(a.b.c, scalafix_2.12), 0.0.1-12-f9db091-SNAPSHOT)]from [IvyRepository(file:/home/user/.ivy2/local/[organisation]/[module]/(scala_[scalaVersion]/)(sbt_[sbtVersion]/)[revision]/[type]s/[artifact](-[classifier]).[ext], dropInfoAttributes = true), MavenRepository(https://repo1.maven.org/maven2), MavenRepository(https://oss.sonatype.org/content/repositories/public), MavenRepository(https://oss.sonatype.org/content/repositories/snapshots)]

which I expect this proposed change to resolve. If this change is accepted, I will update the scala-steward running docs to explain how adding export COURSIER_REPOSITORIES="ivy2Local|central|somePrivateRepository" before running scala-steward allows applying privately hosted scala-fix rules.

@scottrice10
Copy link
Contributor Author

scottrice10 commented Aug 25, 2020

I'll add, this would be for running scalafix rules using the dependency schema, i.e.:

scalafix dependency:someRule@com.org:scalafix:0.0.1

I also tried using the http / github schemas to apply privately hosted scalafix rules, and ran into a different blocker. You can use the http method with privately hosted code following these steps:

  1. While logged into Github, in the browser go the one of the rule files, such as https://github.com/org/repo/blob/master/scalafix/rules/Imports.scala, click the "Raw" button.
  2. Copy the browser url for the raw option, including the ?token=<sessionToken> at the end.
  3. Run:
    scalafix --rules=https://raw.githubusercontent.com/org/repo/master/rules/Imports.scala?token=<sessionToken>

That token is a session token, though, not a personal access token, so it expires after a few days. You can use a Github personal access token to get raw github files using the api and passing headers, like:

  curl -H 'Authorization: token <personalAccessToken>' \
  -H 'Accept: application/vnd.github.v4.raw' \
  -L https://api.github.com/repos/org/repo/contents/rules/Imports.scala

To make this work, scalafix would need to get updated to accept headers used to fetch the raw github file, and scala-steward's configuration would need to get updated to pass an access token in the headers.

@scottrice10
Copy link
Contributor Author

@bjaglin @mlachkar @olafurpg WDYT?

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution! LGTM 👍 Any objections @mlachkar @bjaglin ?

Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the detailed explanation!

What would be great would be to document & prevent regressions against that in a scripted test that would run a rule available via a non-standard, public resolver declared in COURSIER_REPOSITORIES. I don't know of any though, do you by any chance?

Copy link

@arkban arkban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to using the user's configured resolvers instead of any "hard-coded" resolvers

scalafixResolvers := Seq(
Repository.ivy2Local(),
Repository.central(),
scalafixResolvers := Repository.defaults().asScala.toSeq ++ Seq(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only happen if useCoursier := false (from the SBT docs).

Maybe it makes more sense to just to tap into SBT's default resolvers?

Copy link
Contributor Author

@scottrice10 scottrice10 Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only happen if useCoursier := false (from the SBT docs).

As of now, as you mention, scalafix is bypassing the sbt resolvers, so I don' think the sbt setting useCoursier := false applies to setting the scalafixResolvers. This may come into play if your next suggestion is implemented, though?

Maybe it makes more sense to just to tap into SBT's default resolvers?

For the particular problem I'm trying to solve at Rally, yes, this would work without needing an env variable override. Assuming all scala projects are using the rally-sbt-plugin (private to Rally), which puts https:///ivy-libs-snapshot in globalSettings, and our private scalafix rules are published to the same https:///ivy-libs-snapshot, then using SBT's default resolvers for each repo would work for a tool like scala-steward.

Since scala-steward adds sbt-scalafix to repos on the fly, I could see a scenario where the repos' configured SBT resolvers are not the same as the custom resolver where the scalafix rules are hosted, in which case the env variable would still be desired to add additional custom resolvers.

@scottrice10 scottrice10 force-pushed the coursier-default-resolvers branch from e71fdea to 329ab37 Compare August 31, 2020 17:43
@scottrice10
Copy link
Contributor Author

scottrice10 commented Aug 31, 2020

...via a non-standard, public resolver declared in COURSIER_REPOSITORIES. I don't know of any though, do you by any chance?

I agree this would be a good test. But no, I'm aware of a public non-standard resolver that hosts public scalafix rules. I took a look through the public scalafix rules scala-steward is adding by default, but didn't find any that are publishing to a resolver other than sonatype / Maven central.

I setup the test to just verify that scalafixResolvers is updated with the additional repository .

@scottrice10 scottrice10 force-pushed the coursier-default-resolvers branch 5 times, most recently from 95627cc to 87e9474 Compare September 1, 2020 16:35
@scottrice10 scottrice10 force-pushed the coursier-default-resolvers branch from 87e9474 to e0c68bb Compare September 1, 2020 16:39
@scottrice10
Copy link
Contributor Author

@bjaglin Do you think this test is sufficient?

Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful, thank you for figuring out a way to prevent this from regressing!

Comment on lines +1 to +2
# reload to ensure env variable COURSIER_REPOSITORIES is picked up by scalafixResolvers
> reload
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, is the reload required by sbt-dotenv for the env file to be applied?

@bjaglin bjaglin merged commit 15da225 into scalacenter:master Sep 4, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants