-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[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
base: 2.13.x
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
bad0931
to
2113d40
Compare
LGTM, but adding adding a dependency is a significant change. @SethTisue wdyt? This PR doesn't include the classfiles of For
For |
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. |
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. |
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. |
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 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. |
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?
Note that there's no scala-repl.jar, it's all merged into scala-compiler.jar. We have two options
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 |
Okay.
I've added a commit that does this. I've also updated the PR description to mention it.
(I think it's fine if this never happens.)
As far as I can see, it doesn't matter how the REPL gets launched, I don't see any potential for interaction there.
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.)
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 |
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.
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?
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.
Apparently,
public class UnsupportedOperatingSystemException extends UnsupportedOperationException
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.
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" |
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 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.
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 agree, I jumped the gun on utopian naming.
@som-snytt (leaving the presence or absence of (Please review the whole PR description, actually!) |
So we are adding
@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 |
Oops, I was looking at the OSGi thing. I removed that bit from the PR description.
Okay — I'll submit a separate PR for that (after this one is merged). |
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. |
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.) |
TIL
|
|
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:/home/alice/.config/org.scala-lang.repl2/.scala_history
/Users/Alice/Library/Application Support/org.scala-lang.repl2/.scala_history
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