-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Use all known Scala versions when -DallVersions set #10478
Use all known Scala versions when -DallVersions set #10478
Conversation
Can these interfere with Lines 1040 to 1066 in 75b3a05
|
@@ -72,6 +72,7 @@ project(':open-api').name = 'iceberg-open-api' | |||
|
|||
if (null != System.getProperty("allVersions")) { | |||
System.setProperty("flinkVersions", System.getProperty("knownFlinkVersions")) | |||
System.setProperty("scalaVersions", System.getProperty("knownScalaVersions")) |
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 think this would interfere with a few other places where allVersions
is being used
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.
should we then document why scala is left out and rename allVersions
to avoid people assuming something incorrectly?
for exemple one could assume that ./gradlew -DallVersions=true spotlessApply
will reformat all of the code, but i don't think it would
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.
./gradlew -DallVersions=true spotlessApply
will re-format all the code. Code formatting is independent from the underlying Scala version
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.
thanks for proving my example wrong. i do think the basic idea still holds though. the "all versions" property is not quite "all versions"
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.
the purpose when I added allVersions
was to reflect all module versions that we release but I see how this can cause confusion with Scala versions (which only apply for Flink and Spark).
I still don't think we can blindly just include all Scala versions, because they would only apply across Spark modules and will most likely cause issues with Flink modules when you pass Scala 2.13 when building Flink versions.
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.
would it help if we called this allModules
?
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 fine calling this allModules
(we just need to make sure we properly reflect this in docs and any other place where allVersions
is currently used.
Do you see a specific use case where you'd like to build all modules across all supported scala versions?
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.
For example to check the code compiles correctly, but i understand now this may not be needed and that scala version config is more for CI that for building.
Thanks, will rename to allModules!
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.
Relates to #6167