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

Run Scalafix build migrations using sbt #3133

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

Conversation

DavidGregory084
Copy link
Contributor

@DavidGregory084 DavidGregory084 commented Jul 31, 2023

Tries to fix an issue encountered in #3126 whereby Scalafix build migrations failed because they require SemanticDB to be enabled.

This PR changes the SbtAlg to make use of sbt to run Scalafix migrations.

It does this by writing the required sbt configuration files recursively up to the deepest meta-build found in the project before running the scalafix command in each meta-build, similar to getDependencies.

The Scalafix CLI is still used to run syntactic migrations, because sbt-scalafix cannot run migrations on *.sbt files.

Thanks to my employer @opencastsoftware for sponsoring this work.

@DavidGregory084
Copy link
Contributor Author

@fthomas is it possible to dry run Scala Steward (i.e. not open any PRs)? I'd like to test this out but I don't really want to open PRs against other folks' projects.

@DavidGregory084

This comment was marked as outdated.

@fthomas
Copy link
Member

fthomas commented Jul 31, 2023

is it possible to dry run Scala Steward (i.e. not open any PRs)?

There is some kind of dry run (#1828) but that would also not run any Scalafix migrations. You could temporarily comment this line to not push any commits and not creating PRs.

@fthomas
Copy link
Member

fthomas commented Jul 31, 2023

What I also use sometimes for testing is a test repo (like https://github.com/scala-steward-org/test-repo-2) with multiple branches. If Scala Steward creates a PR for a specific branch and I want to test my changes again, I just push another branch (without Scala Steward's change) to the repo and point Scala Steward to that new branch.

@DavidGregory084
Copy link
Contributor Author

DavidGregory084 commented Aug 1, 2023

OK, I've tested this now on a few projects with some mixed results:

  • In spotify/scio the sbt-tpolecat migration ran successfully, although it looks like I have some more work to do on the migration rules as wildcard imports from some of the symbols are not being migrated. Gosh how I hate wildcard imports! They make understanding code difficult and it turns out they make migrating it difficult too.
  • In some builds with older (<1.5.x) sbt, I was expecting in to be migrated to / in the main build.sbt, but it didn't seem to work in at least uclid-org/uclid and sdkman/sdkman-candidates. I wonder if this implies that build migrations need to run at the root level to touch build.sbt, in which case there would actually be no difference between sources and build migration logic except that sources migrations would only operate at buildDepth = 0. EDIT: Absolutely not, this ends with scalafix attempting to translate in from ScalaTest too!

@DavidGregory084
Copy link
Contributor Author

it looks like I have some more work to do on the migration rules

I have done some work on this in sbt-tpolecat now and will send a PR to update the migration SHA in scala-steward

In some builds with older (<1.5.x) sbt, I was expecting in to be migrated to / in the main build.sbt, but it didn't seem to work

I have asked the Scalafix experts in the Scalameta Discord about this 🤞 that they have some idea how to fix it

@DavidGregory084
Copy link
Contributor Author

DavidGregory084 commented Aug 2, 2023

OK, so the word is that sbt-scalafix can't currently update *.sbt files because sbt doesn't return those as part of the build's unmanagedSources, and semantic migrations that update *.sbt files aren't supported and have never worked as it's so hard to generate SemanticDB for *.sbt files.

That means we need to continue running syntactic migrations on *.sbt files with the CLI, perhaps also disabling semantic migrations in the CLI invocations as they won't work in any case.

@fthomas
Copy link
Member

fthomas commented Aug 2, 2023

OK, so the word is that sbt-scalafix can't currently update *.sbt files because sbt doesn't return those as part of the build's unmanagedSources, and semantic migrations that update *.sbt files aren't supported and have never worked as it's so hard to generate SemanticDB for *.sbt files.

:-(

That means we need to continue running syntactic migrations on *.sbt files with the CLI, perhaps also disabling semantic migrations in the CLI invocations as they won't work in any case.

You mean via the --syntactic option? Would that have made a difference for the sbt-tpolecat migration or would it just have not run the migration?

@DavidGregory084
Copy link
Contributor Author

You mean via the --syntactic option? Would that have made a difference for the sbt-tpolecat migration or would it just have not run the migration?

I think it would just have not run the migration, avoiding the errors due to missing semanticdb

@DavidGregory084 DavidGregory084 force-pushed the scalafix-build-migrations-semanticdb branch from b5f2cae to be3c89a Compare August 2, 2023 12:49
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04% 🎉

Comparison is base (ea94b76) 91.04% compared to head (be3c89a) 91.08%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3133      +/-   ##
==========================================
+ Coverage   91.04%   91.08%   +0.04%     
==========================================
  Files         162      162              
  Lines        3406     3423      +17     
  Branches      315      312       -3     
==========================================
+ Hits         3101     3118      +17     
  Misses        305      305              
Files Changed Coverage Δ
...a/org/scalasteward/core/buildtool/sbt/SbtAlg.scala 98.83% <100.00%> (+0.26%) ⬆️
.../scalasteward/core/edit/scalafix/ScalafixCli.scala 100.00% <100.00%> (ø)
.../main/scala/org/scalasteward/core/io/FileAlg.scala 100.00% <100.00%> (ø)

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


[*.{scala,sbt}]
indent_size = 2
indent_style = space
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this because it drives me mad when VS Code uses 4 space indent by default and this configures all editors that understand it 😛

Stream.eval(F.delay(dir.walk(maxDepth))).flatMap(Stream.fromBlockingIterator(_, 1)),
Stream.empty.covary[F]
)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that Files.walk throws an exception if the directory does not exist rather than simply returning an empty iterator

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

Successfully merging this pull request may close these issues.

None yet

2 participants