Make os.home a def

[MaciejG604]

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: https://github.com/com-lihaoyi/os-lib/pull/239

[MaciejG604]

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

[lolgab]

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]

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]

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

[lolgab]

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.

[lihaoyi]

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]

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]

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]

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]

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]

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]

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

[MaciejG604]

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]

@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]

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

[sake92]

@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]

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]

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]

@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]

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]

@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]

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

[lihaoyi]

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

[lefou]

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]

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]

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]

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]

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]

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]

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

[lefou]

Thank you, @MaciejG604 !