-
Notifications
You must be signed in to change notification settings - Fork 140
Add a suffix to the script file to avoid conflict with library name #3647
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
base: main
Are you sure you want to change the base?
Conversation
@@ -13,7 +13,7 @@ object AmmUtil { | |||
case i => fileName.take(i) | |||
} | |||
|
|||
(pkg, Name(wrapper)) | |||
(pkg, Name(s"${wrapper}_scalacli_wrapper")) |
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.
We were also thinking about adding a random suffix, but that would make it impossible to run the main class by name. So instead we did a named that should not interfere with any library. This might be breaking so we also wanted to have a fallback to add the suffix if someone is using the old one. @Gedochao any idea where that can be?
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.
Sorry for getting back late.
As for inferring main classes, detecting script main classes and such, the logic is here:
scala-cli/modules/build/src/main/scala/scala/build/Build.scala
Lines 115 to 183 in a4c286f
private def inferredMainClass( | |
mainClasses: Seq[String], | |
logger: Logger | |
): Either[Seq[String], String] = { | |
val scriptInferredMainClasses = | |
sources.inMemory.collect { | |
case Sources.InMemory(_, _, _, Some(wrapperParams)) => | |
wrapperParams.mainClass | |
} | |
.filter(mainClasses.contains(_)) | |
val rawInputInferredMainClasses = | |
mainClasses | |
.filterNot(scriptInferredMainClasses.contains(_)) | |
.filterNot(mainClassesFoundOnExtraClasspath.contains(_)) | |
.filterNot(mainClassesFoundInUserExtraDependencies.contains(_)) | |
val extraClasspathInferredMainClasses = | |
mainClassesFoundOnExtraClasspath.filter(mainClasses.contains(_)) | |
val userExtraDependenciesInferredMainClasses = | |
mainClassesFoundInUserExtraDependencies.filter(mainClasses.contains(_)) | |
def logMessageOnLesserPriorityMainClasses( | |
pickedMainClass: String, | |
mainClassDescriptor: String, | |
lesserPriorityMainClasses: Seq[String] | |
): Unit = | |
if lesserPriorityMainClasses.nonEmpty then { | |
val first = lesserPriorityMainClasses.head | |
val completeString = lesserPriorityMainClasses.mkString(", ") | |
logger.message( | |
s"""Running $pickedMainClass. Also detected $mainClassDescriptor: $completeString | |
|You can run any one of them by passing option --main-class, i.e. --main-class $first | |
|All available main classes can always be listed by passing option --list-main-classes""".stripMargin | |
) | |
} | |
( | |
rawInputInferredMainClasses, | |
scriptInferredMainClasses, | |
extraClasspathInferredMainClasses, | |
userExtraDependenciesInferredMainClasses | |
) match { | |
case (Seq(pickedMainClass), scriptInferredMainClasses, _, _) => | |
logMessageOnLesserPriorityMainClasses( | |
pickedMainClass = pickedMainClass, | |
mainClassDescriptor = "script main classes", | |
lesserPriorityMainClasses = scriptInferredMainClasses | |
) | |
Right(pickedMainClass) | |
case (rawMcs, scriptMcs, extraCpMcs, userExtraDepsMcs) if rawMcs.length > 1 => | |
Left(rawMcs ++ scriptMcs ++ extraCpMcs ++ userExtraDepsMcs) | |
case (Nil, Seq(pickedMainClass), _, _) => Right(pickedMainClass) | |
case (Nil, scriptMcs, extraCpMcs, userExtraDepsMcs) if scriptMcs.length > 1 => | |
Left(scriptMcs ++ extraCpMcs ++ userExtraDepsMcs) | |
case (Nil, Nil, Seq(pickedMainClass), userExtraDepsMcs) => | |
logMessageOnLesserPriorityMainClasses( | |
pickedMainClass = pickedMainClass, | |
mainClassDescriptor = "other main classes in dependencies", | |
lesserPriorityMainClasses = userExtraDepsMcs | |
) | |
Right(pickedMainClass) | |
case (Nil, Nil, extraCpMcs, userExtraDepsMcs) if extraCpMcs.length > 1 => | |
Left(extraCpMcs ++ userExtraDepsMcs) | |
case (Nil, Nil, Nil, Seq(pickedMainClass)) => Right(pickedMainClass) | |
case (Nil, Nil, Nil, userExtraDepsMcs) if userExtraDepsMcs.length > 1 => | |
Left(userExtraDepsMcs) | |
case (rawMcs, scriptMcs, extraCpMcs, userExtraDepsMcs) => | |
Left(rawMcs ++ scriptMcs ++ extraCpMcs ++ userExtraDepsMcs) | |
} | |
} |
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.
However, I am not convinced we should change the wrapper name - not without explicit configuration, at least.
Maybe via directive/flag.
Changing the default behaviour will be an effective change of syntax, not just for inferring the main class name.
i.e. code like this will stop working:
https://gist.github.com/Gedochao/8902375ad97d24d8ed49c9bb33d5bd08
And this feels quite bad to me, since we've had this syntax since forever.
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.
Maybe something like:
//> using script.overrideWrapperName <name>
?
So that the change of wrapper is committed in the code, to make the changed behaviour predictable.
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.
As for better UX, we could think about detecting the cases such as the reproduction from #3595, and then print a warning, suggesting to override the wrapper... Although detecting such a case would likely mean parsing AST, so that's a tougher cookie.
@yadavan88 we discussed this solution with @tgodzik offline. Let me sum up. the TL;DR of it is that the reproduction from #3595 is a We could technically do a workaround behind a directive/flag (also described in an earlier comment), but it would require the user to know he/she needs to use the workaround. We could have a warning suggesting the workaround (or even implicitly adding the suffix to the wrapper), but that would mean looking for conflicts on the classpath (expensive, were we to do it every time a script is invoked), or at the very least parsing the script and looking at the imports (i.e. the Overall, as it turns out, this is a rather difficult issue for the Spree (not the first time, I know). |
Fixes #3595
Note. Could not add fallaback for the runner name