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

Prepare the build for cross compilation by renaming jvm sources #4282

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Sep 19, 2024

Consists of renames and some changes to build.sbt. Moves tests, and cli source files to shared, dynamic source files predominantly to the jam directory (outside some exceptions, etc. needed later for nice cross-compilation, which are kept in shared). Also moves interfaces files to the jvm folder (as they are to be rewritten to scala for Scala Native).

Part of the Scala Native cross compilation effort (#4279) (whichever PR gets merged first, I will be sure to rebase the other one).

@tgodzik
Copy link
Contributor

tgodzik commented Sep 20, 2024

Looks like you need to change a line somewhere:

scala mdoc:file:scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ConsoleScalafmtReporter.scala

@jchyb jchyb force-pushed the scala-native/move-jvm-target-dirs branch from fcf3981 to 3d75ea2 Compare September 20, 2024 10:26
build.sbt Outdated
javaOptions += "-Dfile.encoding=UTF8",
buildInfoPackage := "org.scalafmt.tests",
buildInfoKeys := Seq[BuildInfoKey](
"resourceDirectory" ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

repeating my question from another pr: why is this needed like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that Test / resourceDirectory should still work? I think it should work yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Test / resourceDirectory).value points to scalafmt-tests/jvm/src/test/resources, but in this commit we moved the tests from there to scalafmt-tests/shared/src/test/resources. This BuildInfo value is used for the DiffTests, and we do not want to keep them in the jvm directory, as later we'll add the Scala Native targets (where the same BuildInfo value changed here will work)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like Test / resourceDirectories contains all possible directories. We could use that and search for shared or make this a list?

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 tried making it a list but this ended up being a way bigger change, so now we just filter for the shared directory

@jchyb jchyb force-pushed the scala-native/move-jvm-target-dirs branch 3 times, most recently from d3467dc to f27efa7 Compare September 20, 2024 13:01
build.sbt Outdated
buildInfoPackage := "org.scalafmt.tests",
buildInfoKeys := Seq[BuildInfoKey](
"resourceDirectory" -> (Test / resourceDirectories).value.find(
_.toPath.endsWith(Paths.get("shared") / "src" / "test" / "resources"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

both approaches feel like a hack, especially this src/test/resources part. i think @tgodzik may have meant to look for the entry which contains shared, with the assumption that there's only one such entry so you don't need to specify what comes after shared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then if someone clones scalafmt into some other shared directory, they are going to be very confused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think /Users/shared/scalafmt/..., etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also FormatTest throws an error if it cannot find any test files in the testDir, so it's not like we risk something being overlooked later on if we specify the test file location manually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not an expert in sbt but gpt4 is, and it suggested using (baseDirectory in ThisBuild).value followed by shared, that should be unambiguous.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, does it make sense to add a new project specifically for shared?

Copy link
Contributor Author

@jchyb jchyb Sep 20, 2024

Choose a reason for hiding this comment

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

(baseDirectory in ThisBuild).value points to the top level scalafmt directory, and I need it to point toscalafmt/scalafmt-tests/shared/src/test/resources (so I could write (ThisBuild / baseDirectory).value / "scalafmt-tests" / "shared" / "src" / "test" / "resources", but that is not much different from my original proposal

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, then, more specifically: would something like this work

"resourceDirectory" -> {
  val sharedTests = (tests / baseDirectory).value / "shared"
  (Test / resourceDirectories).value.find(_.toPath.startsWith(sharedTests))
}

here, we explicitly use sbt constructs for everything except the constant shared in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok, sure

@jchyb jchyb force-pushed the scala-native/move-jvm-target-dirs branch from f27efa7 to 3371f1c Compare September 20, 2024 14:43
@jchyb jchyb marked this pull request as ready for review September 20, 2024 14:51
Copy link
Collaborator

@kitbellew kitbellew left a comment

Choose a reason for hiding this comment

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

approved, with a small nitpick :)

build.sbt Outdated
javaOptions += "-Dfile.encoding=UTF8",
buildInfoPackage := "org.scalafmt.tests",
buildInfoKeys := Seq[BuildInfoKey]("resourceDirectory" -> {
val sharedTests = baseDirectory.value.getParentFile / "shared"
Copy link
Collaborator

Choose a reason for hiding this comment

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

great, thank you. do you think it will make sense to put .toPath here and once, instead of the lambda below?

Moves `tests`, and `cli` source files to shared directory,
`dynamic` source files predominantly to the jvm directory and
also moves interfaces files to jvm folder (as they
are to be rewritten for Scala Native).
@jchyb jchyb force-pushed the scala-native/move-jvm-target-dirs branch from 3371f1c to d76ce10 Compare September 20, 2024 15:39
@kitbellew kitbellew merged commit 140c2f7 into scalameta:main Sep 20, 2024
11 checks passed
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.

3 participants