Make os.home a def #239

MaciejG604 · 2023-11-21T10:25:59Z

In some environments the home directory is not configured, thus user.home can be set to e.g. "?" (like in the issue the Scala CLI team has bumped into). See OpenJDK sources

In those cases using e.g. os.pwd throws, since the os.home is also initialized.
This fix allows os.pwd to still be used when user.home is set to something weird.

One example of such environment with user.home set to "?" is running Jenking on CloudBees, the results of trying to run Scala CLI there looked like this, since we heavily use os.pwd in our code base:

11:15:25  Exception in thread "main" java.lang.ExceptionInInitializerError
11:15:25  	at scala.build.Directories.dbPath$$anonfun$2(Directories.scala:24)
11:15:25  	at scala.Option.map(Option.scala:242)
11:15:25  	at scala.build.Directories.dbPath(Directories.scala:24)
11:15:25  	at scala.build.Directories.dbPath$(Directories.scala:10)
11:15:25  	at scala.build.Directories$OsLocations.dbPath(Directories.scala:35)
11:15:25  	...
11:15:25  Caused by: java.lang.IllegalArgumentException: requirement failed: ? is not an absolute path
11:15:25  	at scala.Predef$.require(Predef.scala:337)
11:15:25  	at os.Path.<init>(Path.scala:453)
11:15:25  	at os.Path$.apply(Path.scala:405)
11:15:25  	at os.package$.<clinit>(package.scala:20)
11:15:25  	... 13 more

Pull request: #239

MaciejG604 · 2023-11-21T12:29:12Z

Maybe make os.home a def to satisfy binary compat?
Or could you update Mima?

lolgab · 2023-11-22T12:02:00Z

Maybe make os.home a def to satisfy binary compat? Or could you update Mima?

You can make a private lazy val and then call it from the def.

private lazy val _home = Path(System.getProperty("user.home"))
def home: Path = _home

lihaoyi · 2023-11-22T13:20:15Z

Is there a reason the JVM implementation is just a def but the native implementation is a lazy val? Feels like they should both be lazy vals

MaciejG604 · 2023-11-22T13:43:26Z

You're right, they should both be lazy vals, I just forgot that, there are two implementations.

lolgab · 2023-11-22T14:00:03Z

You're right, they should both be lazy vals, I just forgot that, there are two implementations.

If you update the PR I think we can merge it and release a new version.

lolgab · 2023-11-22T14:03:25Z

os/test/src/OpTests.scala

+    test("pwd when home is not available") {
+      val correctPwd = java.nio.file.Paths.get(".").toAbsolutePath
+      val oldUserHome = System.getProperty("user.home")
+      System.setProperty("user.home", "?")


This test doesn't test the behavior because the package object is already initialized when you call this method.
I tried running it on main and it passes.

Right, then I guess there are no tests present that could capture that, I reverted the changes.

I think having no unit tests are fine. We could go with an integration test in a separate test target that sets the user.home explicitly, but IMO this is a straightforward enough change to land as is

lihaoyi · 2023-11-23T00:09:45Z

Looks like MIMa is complaining still. Can you take a look and see if those errors are legitimate or not? If they're false positives we should whitelist then in the build file

lolgab · 2023-11-23T07:45:18Z

If changing val to def is not binary compatible, Another possible solution can be to return a null home when it is equal to "?". Not great but at least people can still use the library. The crash will not be direct but one NullPointerException away.

sake92 · 2023-11-23T08:28:24Z

The user.home system property is pretty standard property...
Can't you set it to some dummy value like /tmp/whatever if you are not using it ?
Making it lazy just to avoid this one special case doesn't make much sense to me.

lolgab · 2023-11-23T08:45:02Z

The user.home system property is pretty standard property... Can't you set it to some dummy value like /tmp/whatever if you are not using it ? Making it lazy just to avoid this one special case doesn't make much sense to me.

@sake92 What about null? Unfortunately home is not always defined. Not all unix users have a home directory. You notice when you run an application in a FROM scratch docker container for example.

lefou · 2023-11-23T10:04:57Z

I wonder what the expected behavior of any app could be, if there is no home defined. I mean, requesting home without checking for existence, is almost always a sign that an app is expecting one defined and can't handle the case when it is not. Isn't failing the only true option here?

lefou · 2023-11-23T10:11:50Z

Also, isn't it possible when booting an app to always statically set the home property before anything else is loaded? Due to the lazy classloading of the JVM, it should work if accessing any class from the os package is avoided before the static initialization. Yeah, this might result in some ugly Main class code, but that's what it means to handle such rare scenarios in the JVM.(?)

lefou · 2023-11-23T10:14:07Z

Oh, I chimed in and question the whole PR, because I read the trigger word NullPointerException. haha.

MaciejG604 · 2023-11-23T11:00:49Z

Well, failing when home is not defined is a correct choice in my opinion, but the main issue with the present implementation is that calling e.g. os.pwd, which is totally unrelated renders a big chunk of the features useless since the os package object cannot get initialized.
Also, it would be much better if os-lib fails in a sane manner, that allows the user to figure out the solution for the error on their own.
If what I saw was:

Caused by: os.HomeDirError: home.dir property undefined

life would be better.

On the other hand, what's the gain of moving the responsibility to the user adding an ugly fix in the main method, rather than bumping the library version and changing the behaviour of os.home?

lefou · 2023-11-23T12:10:48Z

@MaciejG604 Sorry, I might have missed the point. Your idea is to let os-lib fail late, once a user (or a default argument) accesses the os.pwd variable, right? I'd expect some more tweaks in the home definition to handle that case of an unset or invalidly set java.home variable and produce a nice error message with the cause and clear instructions how to workaround.

Re the concerns of @sake92, I think in an earlier version of the PR where there was a pure def home, MiMa was happy. We could just go with that. What are the strong arguments for persisting home? Is it optimization (avoid re-creation at any use) or correctness (using the exact same value over time)? Could it be solved by making the lazy val also private[this]?

lihaoyi · 2023-11-23T12:16:28Z

I think the current code is fine, we just need to figure out how to make MIMA happy

sake92 · 2023-11-23T13:05:55Z

@lefou I don't have any strong opinions, just thought this was a Jenkins/CI-only case..
But as @lolgab pointed out it is much more common. 😅
So as all of you said.. as long as it is a compatible change it is fine.
Adding an error message would be even better.

lolgab · 2023-11-23T13:15:04Z

I think the current code is fine, we just need to figure out how to make MIMA happy

This is binary compatible and still eager:

  private val _home = scala.util.Try(Path(System.getProperty("user.home")))

  /**
   * The user's home directory
   */
  def home: Path = _home.get

When the code throws it suspends the exception for when the home method is actually used.

lefou · 2023-11-23T13:46:58Z

I think the current code is fine, we just need to figure out how to make MIMA happy

This is binary compatible and still eager:
  private val _home = scala.util.Try(Path(System.getProperty("user.home")))

  /**
   * The user's home directory
   */
  def home: Path = _home.get
When the code throws it suspends the exception for when the home method is actually used.

Interesting. I think, this may come to a surprise. Time-traveling exceptions may be perceived as a code path executed a second time.

What about this?

  private object _home {
    lazy val value = Path(System.getProperty("user.home"))
  }

  def home: Path = _home.value

This has the advantage, that it's still lazy, so it could be worked around in a main by setting the sysprop.

lolgab · 2023-11-23T13:58:58Z

@lefou good point.
value can also be not lazy since the object is lazy already:

  private object _home {
    val value = Path(System.getProperty("user.home"))
  }

  def home: Path = _home.value

EDIT: lazy val is better since otherwise the exception is wrapped into a ExceptionInInitializerError

lefou · 2023-11-23T14:13:01Z

Since we're now initializing os.home in a new place, this could be an opportunity to share it with Path.expandUser. This was previously not possible due to acyclic enforcements.

lihaoyi · 2023-11-24T03:19:41Z

@lolgab is there a reason you wrapped the lazy val value in an enclosing object home? It seems like just having a top-level lazy val homeValue would be sufficient

lolgab · 2023-11-24T08:34:55Z

Adding a private lazy val introduces a binary incompability on Scala 3, while an object passes Mima just fine.

lihaoyi · 2023-11-24T09:26:43Z

Got it. Can we open an issue on the dotty repo? adding private members breaking binary compatibility seems like a language problem

lefou · 2023-11-24T09:30:37Z

I think we should leave a comment in the code, describing why we make it lazy and why it's inside an object, for future maintainer.

lolgab · 2023-11-24T10:26:18Z

I think we should leave a comment in the code, describing why we make it lazy and why it's inside an object, for future maintainer.

Anyways we have tests for all scenarios.
If you make the lazy val a val it breaks because the type of exception is different.
if you remove the object it fails with MiMa error.
But we can add a comment either way.

Got it. Can we open an issue on the dotty repo? adding private members breaking binary compatibility seems like a language problem

I'm not sure it's an actual problem or something that is not a problem but MiMa doesn't know about it.
I'm not even sure if this still happens in Scala 3.3.1 (we are in Scala 3.1.3 here)

lihaoyi · 2023-11-24T10:45:29Z

Can we try upgrading to scala 3.3.1 and seeing if the problem still exists? It should be ok to force 3.3.x since that their official LTS version

We dont need to fix the issue in this PR if it's due to some upstream code, but we should try to at least pinpoint which one is problematic and open an issue

lolgab · 2023-11-24T10:51:25Z

I tried to bump to 3.3.1 and the issue is still reported by MiMa. Maybe if the previous artifact was also published with Scala 3.3.1?
Can't we release this and change it to private lazy val in 0.10 when we also update Scala 3 (assuming it's possible)?

lefou · 2023-11-24T10:59:04Z

Can't we release this and change it to private lazy val in 0.10 when we also update Scala 3 (assuming it's possible)?

+1. It's a new private implementation detail, after all.

lihaoyi · 2023-11-24T13:23:41Z

I'm happy with merging the code as is, but i would like a comment with a link to a ticket either on the mima or on the dotty repo. Even if we don't know the root cause of the bincompat issue now, such a ticket would provide a route for follow on discussion and a path to either fixing or understanding what is a pretty weird workaround

MaciejG604 · 2023-11-27T15:51:22Z

Opened an issue in MiMa. Thank You for working on this!

os/src-jvm/package.scala

os/src-native/package.scala

lefou · 2023-11-27T19:51:15Z

Thank you, @MaciejG604 !

MaciejG604 changed the title ~~Make os.home lazy~~ Make os.home a def Nov 21, 2023

MaciejG604 mentioned this pull request Nov 22, 2023

Workaround for home.dir property not being set VirtusLab/scala-cli#2573

Merged

MaciejG604 force-pushed the make-os.home-lazy branch from cc47d2d to f7a89bf Compare November 22, 2023 12:28

lolgab reviewed Nov 22, 2023

View reviewed changes

Make os.home a def with an intermediate lazy val _home

dc241cd

MaciejG604 force-pushed the make-os.home-lazy branch from f7a89bf to dc241cd Compare November 22, 2023 15:16

lolgab added 3 commits November 23, 2023 15:08

Add test in separate module

7522a61

Add test also for Scala Native

e6db672

Use private object to avoid bin-compat issue

f6de818

Make test fail if os is already initialized

8dd083c

lolgab added 2 commits November 23, 2023 16:25

Make value lazy to avoid have direct exception

0f072d6

Fix nohometest on windows

fd98bfe

MaciejG604 mentioned this pull request Nov 27, 2023

Adding a private lazy val introduces a binary incompability in Scala 3 lightbend-labs/mima#794

Open

MaciejG604 mentioned this pull request Nov 27, 2023

Remove user.home hack when fix gets released in os-lib VirtusLab/scala-cli#2585

Closed

lefou reviewed Nov 27, 2023

View reviewed changes

os/src-jvm/package.scala Show resolved Hide resolved

os/src-native/package.scala Show resolved Hide resolved

lefou added 2 commits November 27, 2023 17:50

Added comments to bincompat workaround

4f09c73

Merge branch 'main' into make-os.home-lazy

8065f06

lefou merged commit c9d444c into com-lihaoyi:main Nov 27, 2023
9 checks passed

lefou added this to the after 0.9.2 milestone Nov 27, 2023

lefou mentioned this pull request Dec 14, 2023

ExceptionInInitializerError from os.list on Java 17 but not Java 19 #242

Closed

mio-19 mentioned this pull request Oct 10, 2024

os.pwd0 unexpected value when compiled with GraalVM and class init at build time. #318

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make os.home a def #239

Make os.home a def #239

MaciejG604 commented Nov 21, 2023 •

edited by lefou

Loading

MaciejG604 commented Nov 21, 2023

lolgab commented Nov 22, 2023

lihaoyi commented Nov 22, 2023 •

edited

Loading

MaciejG604 commented Nov 22, 2023

lolgab commented Nov 22, 2023

lolgab Nov 22, 2023

MaciejG604 Nov 22, 2023

lihaoyi Nov 23, 2023

lihaoyi commented Nov 23, 2023

lolgab commented Nov 23, 2023

sake92 commented Nov 23, 2023

lolgab commented Nov 23, 2023 •

edited

Loading

lefou commented Nov 23, 2023

lefou commented Nov 23, 2023

lefou commented Nov 23, 2023

MaciejG604 commented Nov 23, 2023 •

edited

Loading

lefou commented Nov 23, 2023 •

edited

Loading

lihaoyi commented Nov 23, 2023

sake92 commented Nov 23, 2023

lolgab commented Nov 23, 2023

lefou commented Nov 23, 2023 •

edited

Loading

lolgab commented Nov 23, 2023 •

edited

Loading

lefou commented Nov 23, 2023

lihaoyi commented Nov 24, 2023

lolgab commented Nov 24, 2023

lihaoyi commented Nov 24, 2023

lefou commented Nov 24, 2023

lolgab commented Nov 24, 2023

lihaoyi commented Nov 24, 2023

lolgab commented Nov 24, 2023 •

edited

Loading

lefou commented Nov 24, 2023

lihaoyi commented Nov 24, 2023

MaciejG604 commented Nov 27, 2023

lefou commented Nov 27, 2023

Make os.home a def #239

Make os.home a def #239

Conversation

MaciejG604 commented Nov 21, 2023 • edited by lefou Loading

MaciejG604 commented Nov 21, 2023

lolgab commented Nov 22, 2023

lihaoyi commented Nov 22, 2023 • edited Loading

MaciejG604 commented Nov 22, 2023

lolgab commented Nov 22, 2023

lolgab Nov 22, 2023

Choose a reason for hiding this comment

MaciejG604 Nov 22, 2023

Choose a reason for hiding this comment

lihaoyi Nov 23, 2023

Choose a reason for hiding this comment

lihaoyi commented Nov 23, 2023

lolgab commented Nov 23, 2023

sake92 commented Nov 23, 2023

lolgab commented Nov 23, 2023 • edited Loading

lefou commented Nov 23, 2023

lefou commented Nov 23, 2023

lefou commented Nov 23, 2023

MaciejG604 commented Nov 23, 2023 • edited Loading

lefou commented Nov 23, 2023 • edited Loading

lihaoyi commented Nov 23, 2023

sake92 commented Nov 23, 2023

lolgab commented Nov 23, 2023

lefou commented Nov 23, 2023 • edited Loading

lolgab commented Nov 23, 2023 • edited Loading

lefou commented Nov 23, 2023

lihaoyi commented Nov 24, 2023

lolgab commented Nov 24, 2023

lihaoyi commented Nov 24, 2023

lefou commented Nov 24, 2023

lolgab commented Nov 24, 2023

lihaoyi commented Nov 24, 2023

lolgab commented Nov 24, 2023 • edited Loading

lefou commented Nov 24, 2023

lihaoyi commented Nov 24, 2023

MaciejG604 commented Nov 27, 2023

lefou commented Nov 27, 2023

MaciejG604 commented Nov 21, 2023 •

edited by lefou

Loading

lihaoyi commented Nov 22, 2023 •

edited

Loading

lolgab commented Nov 23, 2023 •

edited

Loading

MaciejG604 commented Nov 23, 2023 •

edited

Loading

lefou commented Nov 23, 2023 •

edited

Loading

lefou commented Nov 23, 2023 •

edited

Loading

lolgab commented Nov 23, 2023 •

edited

Loading

lolgab commented Nov 24, 2023 •

edited

Loading