-
Notifications
You must be signed in to change notification settings - Fork 277
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
Prepare the build for cross compilation by renaming jvm sources #4282
Conversation
Looks like you need to change a line somewhere:
|
fcf3981
to
3d75ea2
Compare
build.sbt
Outdated
javaOptions += "-Dfile.encoding=UTF8", | ||
buildInfoPackage := "org.scalafmt.tests", | ||
buildInfoKeys := Seq[BuildInfoKey]( | ||
"resourceDirectory" -> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
d3467dc
to
f27efa7
Compare
build.sbt
Outdated
buildInfoPackage := "org.scalafmt.tests", | ||
buildInfoKeys := Seq[BuildInfoKey]( | ||
"resourceDirectory" -> (Test / resourceDirectories).value.find( | ||
_.toPath.endsWith(Paths.get("shared") / "src" / "test" / "resources"), |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, sure
f27efa7
to
3371f1c
Compare
There was a problem hiding this 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" |
There was a problem hiding this comment.
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).
3371f1c
to
d76ce10
Compare
Consists of renames and some changes to build.sbt. Moves
tests
, andcli
source files toshared
,dynamic
source files predominantly to the jam directory (outside some exceptions, etc. needed later for nice cross-compilation, which are kept inshared
). 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).