TempPath (<: AutoCloseable) and withTempFile|Dir convenience methods #147

mpollmeier · 2023-01-04T16:20:44Z

Implements #140

TempPath implements AutoCloseable so we can use automatic resource management (e.g. Using) to automatically remove these temporary resources once they're not needed any longer.

We no longer rely on the JVM's deleteOnExit handler, therefor the temp file gets deleted even if JVM never stopps gracefully (e.g. because it's intended to run continually, or the process get's killed from the outside).

os.temp.withFile and os.temp.withDir are convenience methods that create a temporary file or directory, passes them to a given function and remove them immediately after the function completed, even if the function threw an exception.

os.temp.withFile { file =>
  os.write(file, "some content")
}

os.temp.withDir { file =>
  val file = dir / "somefile"
  os.write(file, "some content")
}

mpollmeier · 2023-01-09T13:09:18Z

CI is red because of cross compilation: scala 2.12 doesn't have Using - I guess that's fixable.

Putting that aside, @lefou would you mind commenting on the proposal so far?

lefou

It looks almost good, but I'm a bit undecided about the fact, that we now always return TempPaths and still provide the deleteOnExit option. Something in me calls for a cleaner approach, e.g. unregister the delete-on-exit shutdown hook after we already closed the TempFile. There is also this other thought, that deleting the file/dir might fail (maybe because the resource is locked) and we could fall-back to the deleteOnExit strategy once we know we failed in the close method. But these thoughts are still a bit wishy-washy, so let me overthink it a bit longer and tell my what you think.

os/src/TempOps.scala

lefou · 2023-01-10T21:38:10Z

Also, shouldn't we in withTempFile and withTempDir provide the same set of options as we do in the apply methods?

mpollmeier · 2023-01-28T17:21:15Z

shouldn't we in withTempFile and withTempDir provide the same set of options as we do in the apply methods?

Done as well

mpollmeier · 2023-01-28T17:24:50Z

Re your other thoughts with "TempPaths and still provide the deleteOnExit option" etc. - I don't have a strong opinion either way, but would prefer to keep it simple. Automagically unregistering the deletion sounds like opening a can of worms, and we may get surprised by how many edge cases something as simple as this can give us...

mpollmeier · 2023-01-28T17:29:03Z

Re supporting scala versions - does mill have standard src dirs like sbt's src/main/scala-2.13 and src/main/scala-3 etc? Do you have an idea on how to handle that best for this build?

lefou · 2023-01-29T18:57:54Z

Re supporting scala versions - does mill have standard src dirs like sbt's src/main/scala-2.13 and src/main/scala-3 etc? Do you have an idea on how to handle that best for this build?

Yeah, Mill's CrossScalaModule does support these source dirs by default, but in this build, the sources is already overridden, to support platform specific sources like src-jvm.

os-lib/build.sc

Lines 146 to 149 in 224aae7

    
           override def sources = T.sources( 
        
             millSourcePath / "src", 
        
             millSourcePath / s"src-$platformSegment" 
        
           )

We can further adapt that to our needs.

mpollmeier · 2023-01-30T08:42:44Z

We can further adapt that to our needs.

You're much more familiar with mill than I am - can you please setup the build for a few scala versions (say _2.13 and _3) and I add the missing code to make it work?

lefou · 2023-02-08T22:06:01Z

We can further adapt that to our needs.

You're much more familiar with mill than I am - can you please setup the build for a few scala versions (say _2.13 and _3) and I add the missing code to make it work?

I added some source directories that support lower or upper Scala version bounds. This is based on ZincWorkerUtil.versionRanges:

https://github.com/com-lihaoyi/mill/blob/7eb39a83974122bfa18892454e1b5aee7177cfca/scalalib/api/src/mill/scalalib/api/ZincWorkerUtil.scala#L128-L136

You can list all source dirs with mill showNamed __.sources.

If you also need exact versioned source dirs, just include super.sources() in your list.

mpollmeier · 2023-02-11T16:19:41Z

Thank you, that worked!

I tried to split up the functionality from object temp into src (for the universally working parts), src-2.13+ (for whatever relies on Using) and src-2.12- (with a simpler try-catch based implementation). Unfortunately I ran into all sorts of problems...
There obviously can't be multiple object temp, so that didn' work. I then tried to follow the object foo extends Function1[Path => A, A] pattern, but I didn't find a way to get that type parameter A to work.

Finally I reverted to a much simpler alternative: since it's concise and self-contained, I just copied scala.util.Using into our build, and changed the package to os.util to avoid any clashes in dowstream projects.

Locally, ./mill -i -k __.test passes all tests but one, which is unrelated IMO...

test.os.ExampleTests test.os.ExampleTests.Source code line length does not exceed 100

mpollmeier · 2023-02-17T15:30:12Z

About those CI failures - I wasn't convinced first, but they're actually legit...

For some reason, calling os.remove.all(this):

class TempPath private[os] (wrapped: java.nio.file.Path)
  extends Path(wrapped) with AutoCloseable {
  override def close(): Unit = os.remove.all(this)
}

fails in Scala 2, with rather obscure compiler errors:

[error] Unwanted cyclic dependency
[info] symbol: object all
[info] symbol: value up
[info] More dependencies at lines 394 394 71 87 71 152 152 87
[info] /home/mp/Projects/os-lib/os/src-jvm/package.scala:13:34:
[info]   def resource(implicit resRoot: ResourceRoot = Thread.currentThread().getContextClassLoader) = {
[info]                                  ^
[info] symbol: trait ResourceRoot
[info] More dependencies at lines 14 14
[info] /home/mp/Projects/os-lib/os/src-jvm/ResourcePath.scala:19:49:
[info]     extends BasePathImpl with ReadablePath with SegmentedPath {
[info]                                                 ^
[info] symbol: trait SegmentedPath
[info] More dependencies at lines 44 36 18 31 44 18 18 31
[error] 6 errors found

I fixed that by reimplementing deleteRecursively, and then uglifying it so that Scala 2.11 handles it as well...
Also shortened the line lengths so that test.os.ExampleTests is happy.
For me locally it's all green now:

./mill -i -k __.clean
./mill -i -k __.test

Can you approve the PR run once more?

lefou

I'm not that happy with coyping the whole scala.util.Using into this project. We actually only want a rough replacement for Using.resource for internal use in case it is not provided (Scala 2.12-), which can be implemented for older Scala versions with a try-finally block. (For that we can forward the implementation of withTempFile to a version specific class/object.) We should not provide a full user API for convenient resource management.

Scala source code also has a different license (Apache-2) than os-lib (MIT), so copying without any additional attribution is a no go.

SethTisue · 2023-02-17T19:01:02Z

fwiw, scala-collection-compat has a backport of Using: scala/scala-collection-compat#319

…e methods Convenience methods that creates a temporary file|dir, passes it them to a given function and remove them immediately after the function completed, even if the function threw an exception. This doesn't rely on the JVM's `deleteOnExit` handler, therefor the temp file gets deleted even if JVM never stopps gracefully (e.g. because it's intended to run continually, or the process get's killed from the outside). ```scala ---- withTempFile { file => os.write(file, "some content") } withTempDir { file => val file = dir / "somefile" os.write(file, "some content") } ``` implements com-lihaoyi#140

A version with a `-` suffix targets code for all Scala versions smaller or equal. A version with a `+` suffix targets code for all Scala versions greater or equal. You can run `mill showNamed __.sources` to see all source dirs.

to avoid any clashes in dowstream projects.

For some reason, calling `os.remove.all(this)`: ```scala class TempPath private[os] (wrapped: java.nio.file.Path) extends Path(wrapped) with AutoCloseable { override def close(): Unit = os.remove.all(this) } ``` fails in Scala 2, with rather obscure compiler errors: ``` [error] Unwanted cyclic dependency [info] symbol: object all [info] symbol: value up [info] More dependencies at lines 394 394 71 87 71 152 152 87 [info] /home/mp/Projects/os-lib/os/src-jvm/package.scala:13:34: [info] def resource(implicit resRoot: ResourceRoot = Thread.currentThread().getContextClassLoader) = { [info] ^ [info] symbol: trait ResourceRoot [info] More dependencies at lines 14 14 [info] /home/mp/Projects/os-lib/os/src-jvm/ResourcePath.scala:19:49: [info] extends BasePathImpl with ReadablePath with SegmentedPath { [info] ^ [info] symbol: trait SegmentedPath [info] More dependencies at lines 44 36 18 31 44 18 18 31 [error] 6 errors found ```

mpollmeier · 2023-03-14T08:19:09Z

Sorry about the silence! I thought it was ok because the attribution (copyright, license) are in the file, but yes I agree, it's better to have less code either way.

Anyway, moving forward: I deleted the copied Using.scala and instead pull in scala-collection-compat, only for Scala 2.11 and 2.12.
This PR is now also rebased onto latest main.
./mill -i -k __.test is green for me locally.

mpollmeier · 2023-03-14T15:39:23Z

Hmm, we seem to have some flakey tests in there, parts of test.os.ExampleTests fail from time to time. This doesn't seem related to this PR, the same happens with main for me locally, and apparently also on CI for other builds, e.g. https://github.com/com-lihaoyi/os-lib/actions/runs/4352680832/jobs/7605762397

Any idea what's going on?

mpollmeier · 2023-03-15T07:50:24Z

I'm locally running ./mill -i -k __.test on the main branch in a loop and the results are very flakey:

grep target build-output.txt | grep ' failed$'
1 targets failed
2 targets failed
1 targets failed
1 targets failed
3 targets failed
2 targets failed
3 targets failed
1 targets failed
1 targets failed
1 targets failed
2 targets failed
1 targets failed
1 targets failed
3 targets failed
2 targets failed
2 targets failed
1 targets failed
3 targets failed
1 targets failed
1 targets failed
2 targets failed
4 targets failed
2 targets failed
1 targets failed
1 targets failed
1 targets failed
2 targets failed
1 targets failed
1 targets failed
2 targets failed
1 targets failed
3 targets failed
2 targets failed
3 targets failed
2 targets failed
1 targets failed
3 targets failed

mpollmeier · 2023-03-15T08:35:49Z

Since this flakeyness is happening on main, I was curious and tested past tags as well. Even if I go back to 0.4.0 it's flakey - at least for me locally, tested on linux with openjdk 11.0.18.

Now I'm wondering: is it just me or what's going on?

lefou · 2023-04-04T06:53:47Z

@mpollmeier Thanks for your analysis. Yeah, the test suite seems flaky. Sometimes it's hard to test filesystem stuff on OSes you don't have, and especially filesystems tend to behave strange/differently in virtualized environments.

About this PR, I hope I can review/edit/merge it soon. Sorry for the delay.

os/src/TempOps.scala

Co-authored-by: Florian Schmaus <flo@geekplace.eu>

SethTisue · 2024-05-20T22:25:18Z

CI seems stuck?

lefou reviewed Jan 10, 2023

View reviewed changes

os/src/TempOps.scala Outdated Show resolved Hide resolved

os/src/TempOps.scala Outdated Show resolved Hide resolved

mpollmeier requested a review from lefou February 14, 2023 12:39

mpollmeier force-pushed the michael/with-temp-file branch from f0c6bf3 to ab3e5ad Compare February 17, 2023 15:27

lefou reviewed Feb 17, 2023

View reviewed changes

mpollmeier force-pushed the michael/with-temp-file branch 2 times, most recently from c32b13a to ee49446 Compare March 14, 2023 08:09

mpollmeier and others added 12 commits March 14, 2023 09:11

return A directly, rather than Try[A] (PR comment)

6bfa0a1

rename to os.temp.with[File|Dir]

4ea3d1f

provide the same set of options as we do in the apply methods

4b16482

Added Scala version specific source paths based on ranges

9a35dba

A version with a `-` suffix targets code for all Scala versions smaller or equal. A version with a `+` suffix targets code for all Scala versions greater or equal. You can run `mill showNamed __.sources` to see all source dirs.

add new modifier for scala 2

40c2f1d

add scala.util.Using from stdlib for 2.12- builds

76be191

change namespace of scala.util.Using

aea113b

to avoid any clashes in dowstream projects.

ugliness for Scala 2.11 support

e2da3fd

reducing line length to <= 100 chars - I miss scalafmt

e93cbdc

use scala-collection-compat for scala 2.12-

0b8bfef

mpollmeier force-pushed the michael/with-temp-file branch from ee49446 to 0b8bfef Compare March 14, 2023 08:11

Merge branch 'main' into michael/with-temp-file

c2dfe01

Flowdalic reviewed Oct 10, 2023

View reviewed changes

os/src/TempOps.scala Outdated Show resolved Hide resolved

Flowdalic reviewed Oct 10, 2023

View reviewed changes

os/src/TempOps.scala Outdated Show resolved Hide resolved

mpollmeier and others added 2 commits November 22, 2023 15:26

Update os/src/TempOps.scala

6edb4f2

Co-authored-by: Florian Schmaus <flo@geekplace.eu>

Update os/src/TempOps.scala

7e6c994

Co-authored-by: Florian Schmaus <flo@geekplace.eu>

lefou force-pushed the michael/with-temp-file branch from 81786a4 to 7e6c994 Compare May 15, 2024 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TempPath (<: AutoCloseable) and withTempFile|Dir convenience methods #147

TempPath (<: AutoCloseable) and withTempFile|Dir convenience methods #147

mpollmeier commented Jan 4, 2023 •

edited

mpollmeier commented Jan 9, 2023

lefou left a comment

lefou commented Jan 10, 2023

mpollmeier commented Jan 28, 2023

mpollmeier commented Jan 28, 2023

mpollmeier commented Jan 28, 2023

lefou commented Jan 29, 2023

mpollmeier commented Jan 30, 2023

lefou commented Feb 8, 2023 •

edited

mpollmeier commented Feb 11, 2023 •

edited

mpollmeier commented Feb 17, 2023

lefou left a comment

SethTisue commented Feb 17, 2023

mpollmeier commented Mar 14, 2023

mpollmeier commented Mar 14, 2023

mpollmeier commented Mar 15, 2023

mpollmeier commented Mar 15, 2023

lefou commented Apr 4, 2023

SethTisue commented May 20, 2024

TempPath (<: AutoCloseable) and withTempFile|Dir convenience methods #147

Are you sure you want to change the base?

TempPath (<: AutoCloseable) and withTempFile|Dir convenience methods #147

Conversation

mpollmeier commented Jan 4, 2023 • edited

mpollmeier commented Jan 9, 2023

lefou left a comment

Choose a reason for hiding this comment

lefou commented Jan 10, 2023

mpollmeier commented Jan 28, 2023

mpollmeier commented Jan 28, 2023

mpollmeier commented Jan 28, 2023

lefou commented Jan 29, 2023

mpollmeier commented Jan 30, 2023

lefou commented Feb 8, 2023 • edited

mpollmeier commented Feb 11, 2023 • edited

mpollmeier commented Feb 17, 2023

lefou left a comment

Choose a reason for hiding this comment

SethTisue commented Feb 17, 2023

mpollmeier commented Mar 14, 2023

mpollmeier commented Mar 14, 2023

mpollmeier commented Mar 15, 2023

mpollmeier commented Mar 15, 2023

lefou commented Apr 4, 2023

SethTisue commented May 20, 2024

mpollmeier commented Jan 4, 2023 •

edited

lefou commented Feb 8, 2023 •

edited

mpollmeier commented Feb 11, 2023 •

edited