Skip to content

[NEEDS POLISH] Keep REPL history file in OS-standard location (not at top of user home) #10611

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

Draft
wants to merge 2 commits into
base: 2.13.x
Choose a base branch
from

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Nov 23, 2023

Use the directories-jvm library to determine where REPL history is kept, rather than always using (some would say polluting) the top level of the user's home directory.

Formerly, ~/.scala_history_jline3 was always used. The history file will now typically be found in:

  • Linux: /home/alice/.config/org.scala-lang.repl2/.scala_history
  • Mac: /Users/Alice/Library/Application Support/org.scala-lang.repl2/.scala_history
  • Windows: C:\Users\Alice\AppData\Roaming\org.scala-lang\repl2\config\.scala_history

Note that this PR adds the directories-jvm library (which is Java-based, not Scala-based, and only about 14K) as a dependency of scala-compiler JAR.

Fixes scala/bug#12627

@scala-jenkins scala-jenkins added this to the 2.13.13 milestone Nov 23, 2023
@lrytz

This comment was marked as resolved.

@lrytz lrytz requested a review from SethTisue November 28, 2023 14:11
@SethTisue SethTisue self-assigned this Nov 28, 2023
@som-snytt

This comment was marked as resolved.

@som-snytt

This comment was marked as resolved.

@SethTisue

This comment was marked as resolved.

@som-snytt

This comment was marked as resolved.

@som-snytt som-snytt force-pushed the issue/12627-repl-dir branch from bad0931 to 2113d40 Compare November 30, 2023 23:05
@lrytz
Copy link
Member

lrytz commented Dec 1, 2023

LGTM, but adding adding a dependency is a significant change. @SethTisue wdyt?

This PR doesn't include the classfiles of directories into scala-compiler.jar, but it adds a dependency to the compiler's pom.

For scala-asm and java-diff-utils we include the classfiles into scala-compiler.jar (https://github.com/scala/scala/blob/v2.13.12/build.sbt#L503-L504). For asm we also remove the dependency from the pom, I guess it's an oversight that we don't do so for java-diff-utils (https://github.com/scala/scala/blob/v2.13.12/build.sbt#L566)?

java-diff-utils was included in scala-compiler.jar (and also added as a dependency in the pom) not so long ago, only in 2.13.9 (#9987). It's good to see that nothing broke 🙂.

For jna and jline we keep them as ordinary dependencies in the pom. Maybe that has something to do with sbt and how it provides jline to the console, I don't know...

@som-snytt
Copy link
Contributor Author

I didn't think of the sbt angle. I was following jline just because. It's only a couple of classes, so ingestion is feasible. In summary, the feature (improved file location) is nice to have at zero cost; and also the PR is a probe of whether or how to incorporate a relatively trivial dependency. I resisted giving much thought to either issue.

@SethTisue
Copy link
Member

I'll think on this next week.

My initial gut reaction is that it's okay because it's only for the REPL, and anyway it's a Java-based dependency.

If we take it, we need to make sure and put the license information (MPL) in the usual places.

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Dec 1, 2023
@SethTisue
Copy link
Member

One thought I have is that it ought to happen in Scala 3 first. Because if it's not going to also happen (in a reasonably short time) in Scala 3, then it isn't worth disrupting the Scala 2 status quo? But if does happen in Scala 3, we can argue that it's worth aligning with.

@som-snytt
Copy link
Contributor Author

Unfortunately, the previous theory or policy, that Scala 3 would innovate and then we can backport, doesn't work because Scala 3 has its own priorities and resource limitations. It makes more sense to try out modest innovations on Scala 2 and then forward port what is useful (in the form of a fully baked design). I don't know that -Wconf turned out that way because of the difference in the feature set (but maybe it's the exception proving the rule), but -Wnonunit-statement is a clear example of a feature that benefited from experimentation and also proved useful enough (to someone) to port.

The innovation that would affect Scala 3 directly would be: share the history file! Use a custom history that knows the source version for each line. That might even be helpful for 3.x versions. Now that you mention it, why isn't repl history in tasty format? The use case is that I want to try something out in Scala 2, then switch to 3 and retry it. Or rather, conversely! It was also be cool if there were a rewrite or quickfix so when Scala 2 runs a line of Scala 3 syntax, I don't have to edit anything! You see what happens when you enable innovation.

@lrytz
Copy link
Member

lrytz commented Dec 5, 2023

I agree we don't need to block everything on Scala 3 parity, we can decide case by case. Here it's fine if we go ahead.

Seth also mentioned that scala-cli would eventually become the REPL launcher, does that doesn't affect the history file?

My initial gut reaction is that it's okay because it's only for the REPL

Note that there's no scala-repl.jar, it's all merged into scala-compiler.jar. We have two options

  • a new dependency in the compiler pom (current state of this PR), or
  • merge the classfiles into scala-compiler.jar

Not sure what assumption there are these days in build tools about the jar files of a Scala version. Maybe including the classes into the compiler jar is safer. On the other hand, without shading, if a project has a dependency on scala-compiler they don't have a way to pick a different version of directories or java-diff-utils.

@SethTisue
Copy link
Member

I agree we don't need to block everything on Scala 3 parity, we can decide case by case. Here it's fine if we go ahead.

Okay.

we need to make sure and put the license information (MPL) in the usual places

I've added a commit that does this. I've also updated the PR description to mention it.

The innovation that would affect Scala 3 directly would be: share the history file!

(I think it's fine if this never happens.)

Seth also mentioned that scala-cli would eventually become the REPL launcher, does that doesn't affect the history file?

As far as I can see, it doesn't matter how the REPL gets launched, I don't see any potential for interaction there.

Maybe including the classes into the compiler jar is safer

In some sense yes. But at the cost of extra build hassle for us, and also at the cost of evading the normal mechanism for dependencies. I don't think shading is a clear win in this case. (And note that we've even discussed going in the other direction with e.g. ASM, which we shade because we have un-upstreamed changes. I think there's a scala-dev ticket about addressing that.)

Not sure what assumption there are these days in build tools about the jar files of a Scala version

Well, I don't remember any trouble around this when we've changed or updated the REPL dependencies in the past. (It's possible some tooling needed to adapt downstream, but if so it happened without ).

I've attached the release-notes label partly with the intent of alerting tooling maintainers. (It also alerts other folks for whom a new dependency may create work, e.g. shops that use internal artifact repositories to which they vet all additions, rather than allowing their developers direct access to Maven Central.)

val projectdirs = ProjectDirectories.from("org", "scala-lang", "repl2")
projectdirs.dataLocalDir
} catch {
case _: UnsupportedOperationException => userHome
Copy link
Member

Choose a reason for hiding this comment

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

Under what circumstances do we anticipate UnsupportedOperationException potentially occurring?

Related question: what happens if directories-jvm is, due to some mishap, not present on the classpath? Does the behavior degrade gracefully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently,

public class UnsupportedOperatingSystemException extends UnsupportedOperationException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the lib not just making strings but also running shells to probe for dirs? I can't tell by looking, I just shifted back to an early morning schedule and won't be awake for another couple of hours.

Offhand, I would prefer not to spawn any (more) processes. Perhaps I was lazy in my due diligence.

} catch {
case _: UnsupportedOperationException => userHome
}
val historyFile = s"$datadir/.scala_history"
Copy link
Member

@SethTisue SethTisue Jan 18, 2024

Choose a reason for hiding this comment

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

I'm dubious about omitting the _jline3 here. Maybe it's okay, but I would need convincing.

We added it when we moved from JLine 2 to JLine 3 because the format is different. So there is a risk of JLine 2 failing to understand the JLine 3 format, and vice versa. Perhaps even a risk of corruption.

Do we believe that it can be omitted because the UnsupportedOperationException will never occur in practice and thus the file will always be in a different directory than any history file associated with a Scala version that uses JLine 2...? Seems dubious.

Finally, Scala 2 generally is in a "don't fix it if ain't broke" kind of state, and since we've been using .scala_history_jline3 for a while now, I'm inclined to leave it alone unless convinced otherwise.

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 agree, I jumped the gun on utopian naming.

@SethTisue
Copy link
Member

SethTisue commented Jan 18, 2024

@som-snytt (leaving the presence or absence of _jline3 aside for the moment) does the "The history file will now typically be found in..." section I added to the PR description look correct to you for Windows and Linux? I tested it the Mac one on my own laptop.

(Please review the whole PR description, actually!)

@SethTisue SethTisue changed the title Use directories to locate repl history Keep REPL history file in OS-standard location (not at top of user home) Jan 18, 2024
@SethTisue SethTisue removed their assignment Jan 18, 2024
@lrytz
Copy link
Member

lrytz commented Jan 18, 2024

So we are adding directories as a pom dependency.

The new dependency listed in the POM as "optional"

@SethTisue I don't see that (there's something about "optional" in osgi). I don't think we ever used "optional" in our poms, so we shouldn't.

Let's also clean up the inconsistency around java-diff-utils (#10611 (comment)), i.e., stop merging its classfiles into scala-compiler.jar.

@SethTisue
Copy link
Member

SethTisue commented Jan 18, 2024

I don't think we ever used "optional" in our poms, so we shouldn't.

Oops, I was looking at the OSGi thing. I removed that bit from the PR description.

Let's also clean up the inconsistency around java-diff-utils (#10611 (comment)), i.e., stop merging its classfiles into scala-compiler.jar.

Okay — I'll submit a separate PR for that (after this one is merged).

@som-snytt
Copy link
Contributor Author

IIRC I intended this as a baby step toward better file management, with an eventual goal of probing what it takes for the new dependency and also improving interop between versions with a custom history.

I haven't followed up with a custom history; that would be experimental. The use case is I want to try something with both 2&3 to see if it even works the same. "But how often do you do that?"

It was not a goal to create a time suck for Seth, so we could skip the feature for now. But if you're in for a penny, I'm in for a pound.

@SethTisue
Copy link
Member

SethTisue commented Jan 18, 2024

I do think it's a good change and worth pursuing.

(Though whether it makes 2.13.13 or has to wait for 2.13.14 doesn't concern me. I will provisionally milestone it for 2.13.14.)

@SethTisue SethTisue modified the milestones: 2.13.13, 2.13.14 Jan 22, 2024
@SethTisue SethTisue marked this pull request as draft January 22, 2024 03:38
@som-snytt
Copy link
Contributor Author

TIL

  // Where we keep fsc's state (ports/redirection)
  lazy val scalacDir = (Path(Properties.userHome) / ".scalac").createDirectory(force = false)

@SethTisue SethTisue modified the milestones: 2.13.14, 2.13.15 Mar 13, 2024
@som-snytt som-snytt closed this Mar 20, 2024
@SethTisue SethTisue removed this from the 2.13.15 milestone Mar 21, 2024
@som-snytt som-snytt changed the title Keep REPL history file in OS-standard location (not at top of user home) [NEEDS POLISH] Keep REPL history file in OS-standard location (not at top of user home) Jan 27, 2025
@som-snytt som-snytt deleted the issue/12627-repl-dir branch January 27, 2025 03:47
@som-snytt som-snytt restored the issue/12627-repl-dir branch June 24, 2025 21:45
@som-snytt som-snytt reopened this Jun 24, 2025
@scala-jenkins scala-jenkins added this to the 2.13.17 milestone Jun 24, 2025
@som-snytt
Copy link
Contributor Author

[NEEDS POLISH] means he hopes VirtusLab steps in to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't put loose files like .scala_history_jline3 in the root of user dirs
4 participants