Skip to content
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

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mpollmeier
Copy link

@mpollmeier mpollmeier commented Jan 4, 2023

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
Copy link
Author

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?

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
os/src/TempOps.scala Outdated Show resolved Hide resolved
@lefou
Copy link
Member

lefou commented Jan 10, 2023

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

@mpollmeier
Copy link
Author

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

Done as well

@mpollmeier
Copy link
Author

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
Copy link
Author

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
Copy link
Member

lefou commented Jan 29, 2023

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
Copy link
Author

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
Copy link
Member

lefou commented Feb 8, 2023

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
Copy link
Author

mpollmeier commented Feb 11, 2023

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
Copy link
Author

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?

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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

@mpollmeier 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
…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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

lefou commented Apr 4, 2023

@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 Outdated Show resolved Hide resolved
os/src/TempOps.scala Outdated Show resolved Hide resolved
mpollmeier and others added 2 commits November 22, 2023 15:26
Co-authored-by: Florian Schmaus <[email protected]>
Co-authored-by: Florian Schmaus <[email protected]>
@SethTisue
Copy link
Contributor

CI seems stuck?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants