Skip to content

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.

Sign up for GitHub

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

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

Add a suffix to the script file to avoid conflict with library name #3647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b60c936
Add a suffix to the script file to avoid conflict with library name
yadavan88 Apr 24, 2025
be66777
fixed formatting
yadavan88 Apr 24, 2025
ac11acd
Merge branch 'main' into worksheet_name_change
yadavan88 Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/build/src/main/scala/scala/build/internal/AmmUtil.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ object AmmUtil {
case i => fileName.take(i)
}

(pkg, Name(wrapper))
(pkg, Name(s"${wrapper}_scalacli_wrapper"))
Copy link
Member

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?

Copy link
Contributor

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)
}
}

Copy link
Contributor

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.

Copy link
Contributor

@Gedochao Gedochao Apr 29, 2025 •
edited
Loading

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.

Copy link
Contributor

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.

}

def encodeScalaSourcePath(path: Seq[Name]) = path.map(_.backticked).mkString(".")
Expand Down
3 changes: 1 addition & 2 deletions modules/build/src/main/scala/scala/build/preprocessing/ScriptPreprocessor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,7 @@ case object ScriptPreprocessor extends Preprocessor {
inputArgPath.getOrElse(subPath.toString)
)

val className = (pkg :+ wrapper).map(_.raw).mkString(".")
val relPath = os.rel / (subPath / os.up) / s"${subPath.last.stripSuffix(".sc")}.scala"
val relPath = os.rel / (subPath / os.up) / s"${subPath.last.stripSuffix(".sc")}.scala"

val file = PreprocessedSource.UnwrappedScript(
originalPath = reportingPath.map((subPath, _)),
Expand Down
39 changes: 31 additions & 8 deletions modules/build/src/test/scala/scala/build/tests/ScriptWrapperTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,39 @@ class ScriptWrapperTests extends TestUtil.ScalaCliBuildSuite {
)
expect(projectDir.size == 1)
expectClassWrapper(
"script1",
"script1_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script1.scala"
)
expectClassWrapper(
"script2",
"script2_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script2.scala"
)
}
}

test(s"run script file properly when file name equals a library name") {
val inputs = TestInputs(
os.rel / "script1.sc" ->
s"""//> using dep "dev.zio::zio:2.1.16"
|
|import zio.ZIO
|""".stripMargin
)

inputs.withBuild(baseOptions, buildThreads, bloopConfigOpt) {
(root, _, maybeBuild) =>
expect(maybeBuild.orThrow.success)
val projectDir = os.list(root / ".scala-build").filter(
_.baseName.startsWith(root.baseName + "_")
)
expect(projectDir.size == 1)
expectClassWrapper(
"script1_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script1.scala"
)
}
}

for {
useDirectives <- Seq(true, false)
(directive, options, optionName) <- Seq(
Expand Down Expand Up @@ -160,11 +183,11 @@ class ScriptWrapperTests extends TestUtil.ScalaCliBuildSuite {
)
expect(projectDir.size == 1)
expectObjectWrapper(
"script1",
"script1_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script1.scala"
)
expectObjectWrapper(
"script2",
"script2_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script2.scala"
)
}
Expand Down Expand Up @@ -203,11 +226,11 @@ class ScriptWrapperTests extends TestUtil.ScalaCliBuildSuite {
)
expect(projectDir.size == 1)
expectAppWrapper(
"script1",
"script1_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script1.scala"
)
expectAppWrapper(
"script2",
"script2_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script2.scala"
)
}
Expand Down Expand Up @@ -249,11 +272,11 @@ class ScriptWrapperTests extends TestUtil.ScalaCliBuildSuite {
expect(projectDir.size == 1)

expectObjectWrapper(
"script1",
"script1_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script1.scala"
)
expectObjectWrapper(
"script2",
"script2_scalacli_wrapper",
projectDir.head / "src_generated" / "main" / "script2.scala"
)
}
Expand Down
Loading