TempPath (<: AutoCloseable) and withTempFile|Dir convenience methods #147
Conversation
|
CI is red because of cross compilation: scala 2.12 doesn't have Putting that aside, @lefou would you mind commenting on the proposal so far? |
There was a problem hiding this comment.
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.
|
Also, shouldn't we in |
Done as well |
|
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... |
|
Re supporting scala versions - does mill have standard src dirs like sbt's |
Yeah, Mill's Lines 146 to 149 in 224aae7 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 You can list all source dirs with If you also need exact versioned source dirs, just include |
|
Thank you, that worked! I tried to split up the functionality from Finally I reverted to a much simpler alternative: since it's concise and self-contained, I just copied Locally, |
mpollmeier
force-pushed
the
michael/with-temp-file
branch
from
f0c6bf3
to
ab3e5ad
Compare
|
About those CI failures - I wasn't convinced first, but they're actually legit... For some reason, calling 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: I fixed that by reimplementing Can you approve the PR run once more? |
There was a problem hiding this comment.
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.
|
fwiw, scala-collection-compat has a backport of |
mpollmeier
force-pushed
the
michael/with-temp-file
branch
2 times, most recently
from
c32b13a
to
ee49446
Compare
…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
force-pushed
the
michael/with-temp-file
branch
from
ee49446
to
0b8bfef
Compare
|
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 |
|
Hmm, we seem to have some flakey tests in there, parts of Any idea what's going on? |
|
I'm locally running |
|
Since this flakeyness is happening on Now I'm wondering: is it just me or what's going on? |
|
@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. |
Co-authored-by: Florian Schmaus <[email protected]>
Co-authored-by: Florian Schmaus <[email protected]>
lefou
force-pushed
the
michael/with-temp-file
branch
from
81786a4
to
7e6c994
Compare
|
CI seems stuck? |
Implements #140
TempPathimplementsAutoCloseableso 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
deleteOnExithandler, 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.withFileandos.temp.withDirare 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") }