lihaoyi/os-lib

Li Haoyi

@lihaoyi

I think duplicate events is probably fine

utgheith

@utgheith

But it would be more than duplicate events in this case. It might generate "unexpected" events. For example: I interpret the following paragraph as a lower bound but someone that interprets it as strict equality might be confused by seeing DELETE events for sub directories of moved directories:

Once the call to watch returns, onEvent is guaranteed to receive a an event containing the path for:

- Every file or folder that gets created, deleted, updated or moved within the watched folders

- For copied or moved folders, the path of the new folder as well as every file or folder within it.

- For deleted or moved folders, the root folder which was deleted/moved, but without the paths of every file that was within it at the original location

Li Haoyi

@lihaoyi

entirely unexpected events sounds a bit tough

utgheith

@utgheith

But we're already in violation of the 3rd assertion (using the strict interpretation). DELETE events are being generated for deleted sub-folders but not for moved ones.

Li Haoyi

@lihaoyi

yeah

utgheith

@utgheith

I apologize for all the spam but here is a simple test that shows that the problem is in WatchService. os.watch is likely masking some of those issues in its implementation but not all.

https://github.com/utgheith/watch3/blob/master/src/main/scala/main.scala

utgheith

@utgheith

It seems that readme.md is using travis-ci badges instead of the github actions ones. Clicking on the build status goes back to a January build (telling me that the switch occurred around this time). Tried to submit a pull request that fixes that but it gets stuck and never runs the tests. This seems to be related to an issue that I don't fully understand but requires special privileges to deal with anyway. Here is a link:

https://github.community/t/expected-waiting-for-status-to-be-reported/16727

Lorenzo Gabriele

@lolgab

@utgheith The build now has a name: ci that's why the statuses don't match. Can you please rebase the PR on top of current master?

utgheith

@utgheith

done

Li Haoyi

@lihaoyi

@utgheith I would not be surprised by bugs in the underlying WatchService, would be nice if we could paper over it though. Not sure hwo hard that would be

utgheith

@utgheith

How about using intotify directly? Similar to what happens on MacOS. But first I need to convince myself that the issues in WatchService are not cause by inotify :-)

utgheith

@utgheith

com-lihaoyi/os-lib#69 is an attempt to paper over it but it makes me uncomfortable.

Li Haoyi

@lihaoyi

using inotify directly is possible, I never thought of it because I thought WatchService generally worked for the stuff we used it for (as opposed on on OS-X, where WatchService does not work at all
on a large folder due to polling)

utgheith

@utgheith

I will write some stress tests and share the results but I have a few concerns regarding os.watch and I want to know your thoughts on them.
(1) os.watch is the sort of thing that requires stress testing. Unit testing is still important obviously. Do you have your own stress tests? If not, is there value in developing some and running them as part of CI?
(2) The API doesn't make any statements regarding OVERFLOW (silently drop events, force the caller to re-examine the world, ...). inotify uses a finite system-wide queue for notifications and there is no guarantee that it will not overflow.

Li Haoyi

@lihaoyi

idont have any stress tests directly on the library; only for downstream projects.

I actualyl dont know what to do about OVERFLOW events

utgheith

@utgheith

In the one project where I used os.watch, I resorted to periodically restarting the application in an orderly manner. I could imagine writing a loop that reinitializes the world when it receives an OVERFLOW event.

utgheith

@utgheith

Same test as before but uses inotify (not directly but by spawning a process running inotifywatch). Works reliably on the same test that was failing before. This strongly hints that using inotify directly will be more reliable. Also inotify is explicit about how it handles moves (move_to, move_from, move_self, etc)

https://github.com/utgheith/watch4/blob/master/src/main/scala/main.scala

Li Haoyi

@lihaoyi

@utgheith I think com-lihaoyi/os-lib#69 sounds reasonable; am I right to say that this change is linux-only and won't affect the fsevents-based OSX version?

if someone doesn't care about the inner events, they can just ignore them

I'm happy to have a raw inotify-based implementation too, if you don't mind wrangling some JNA FFI code. That's what we ended up doing for the OS-X backend anyway so it's not unprecedented

Li Haoyi

@lihaoyi

w.r.t. the extra events, the only worry I can think of is if moving a large folder results in a huge flood of useless events

hard to guess whether that would be a problem in practice or not; for our use case at work, it'll probably just take a bit longer to realize it doesn't need to do anything, but no big deal

utgheith

@utgheith

Started an intofiy based Watcher implementation (com-lihaoyi/os-lib#70). It's still rough around the edges and didn't have a change to stress-test it but it already passes the existing os.watch tests reliably (even on Docker). Would be great if you took look at the overall design.

@lihaoyi yes, the changes will be Linux only either way because OSX uses its own Watcher

Li Haoyi

@lihaoyi

OSX has similar bugs :p also only under heavy load, so havent had a chance to investigate too deeply

utgheith

@utgheith

@lihaoyi your assertion is correct. I added a simple randomized test that creates trees with up to 100 things in it then delete them. The bad news: it fails on both OSX and Linux. The good news: it passes for the inotify based Watcher. I think this will be sorted out.

Do you have any objections to adding scalacheck as a dependency? It makes working with such tests easier.

Li Haoyi

@lihaoyi

go for it

utgheith

@utgheith

com-lihaoyi/os-lib#70 is ready for review. The randomized tests fail with fairly high probability for both OSX and Linux when using the WatchServiceWatcher. They are currently disabled for OSX.

Li Haoyi

@lihaoyi

will take a look

utgheith

@utgheith

I noticed that this pattern repeats in the code. I think having both the overloaded method and the default value is redundant and confusing. Is there some subtle reason for doing this? I could see scenarios in which having the overloaded method would be more efficient. Why have both? Mostly curious.

def apply(src: Path, sort: Boolean = true) = { ... }
def apply(src: Path) = apply(src, true)

Li Haoyi

@lihaoyi

@utgheith it's to make things like Seq[os.Path](...).map(thing) work, otherwise we'd need to do .map(thing(_)) which is kinda annoying

utgheith

@utgheith

But does that require the default value in the long form?

Tobias Roeser

@lefou

@lihaoyi, I'd like to tag current master as 0.7.5 to make latest changes available downstream. Any objections?

I would create a new commit of course, to update the Readme and make CI release process happy.

Li Haoyi

@lihaoyi

go for it

Tobias Roeser

@lefou

Done

discobaba

@uncleweirdo_twitter

how do you do the equivalent of "chown -h <whoever> <file>", i.e. don't follow symlink?

discobaba

@uncleweirdo_twitter

def setLinkUserAndGroup(link: os.Path, usr: String, grp: String) : Unit = {
  val lookupService = FileSystems.getDefault.getUserPrincipalLookupService
  val attrView = Files.getFileAttributeView(Paths.get(link.toString()), classOf[PosixFileAttributeView],
    LinkOption.NOFOLLOW_LINKS)
  attrView.setOwner(lookupService.lookupPrincipalByName(usr))
  attrView.setGroup(lookupService.lookupPrincipalByGroupName(grp))
}

or something, I guess. I feel your pain.

Tobias Roeser

@lefou

Hi, I introduced a binary incompatible change in os-lib 0.7.5 which I'm going to fix with PR #80. I'm sorry for the inconveniences. If this PR is merged, I'd like to cut a new os-lib release. Any review appreciated.

Tobias Roeser

@lefou

Tagged 0.7.8

Jack Koenig

@jackkoenig

Does .call(check = false) not work?

$ amm
Loading...
Welcome to the Ammonite Repl 2.4.0 (Scala 2.13.6 Java 11.0.11)
@ os.proc("foo").call(check = false) 
java.io.IOException: Cannot run program "foo" (in directory ".../oslib"): error=2, No such file or directory
  java.lang.ProcessBuilder.start(ProcessBuilder.java:1128)
  java.lang.ProcessBuilder.start(ProcessBuilder.java:1071)
  os.proc.proc$lzycompute$1(ProcessOps.scala:128)
  os.proc.proc$1(ProcessOps.scala:122)
  os.proc.spawn(ProcessOps.scala:135)
  os.proc.call(ProcessOps.scala:76)
  ammonite.$sess.cmd0$.<clinit>(cmd0.sc:1)
java.io.IOException: error=2, No such file or directory
...

Jack Koenig

@jackkoenig

My expectation being it would not throw an exception--I'd like to check the exit code. Looking at the code it seems like it's not supposed to ever throw an IOException so something is going on here: https://github.com/com-lihaoyi/os-lib/blob/790b81aba491047b73dd481e290abb36ee1a7de8/os/src-jvm/ProcessOps.scala#L84

Jack Koenig

@jackkoenig

Actually looking at that stack trace, yeah it looks like proc.spawn is accidentally throwing the exception here: https://github.com/com-lihaoyi/os-lib/blob/790b81aba491047b73dd481e290abb36ee1a7de8/os/src-jvm/ProcessOps.scala#L128 I'll file an issue

jodersky

@jodersky

@jackkoenig, does "foo" exist on your path? I think that what you're seeing is expected behavior: check = false will not throw if the exit code is non zero, but it does not influence what happens if the executable doesn't exist in the first place

here is the result of some tests:

scala> os.proc("false").call() // false returns 1, so this should throw
os.SubprocessException: CommandResult 1

  at os.SubprocessException$.apply(Model.scala:202)
  at os.proc.call(ProcessOps.scala:85)
  ... 28 elided

scala> os.proc("false").call(check = false) // error is suppressed                                                                                                                                                                                                      
val res4: os.CommandResult = CommandResult 1

scala> os.proc("nonexistant").call(check = false) // we still get an error, because "nonexistant" is does not exist                                                                                                                                                                                                    
java.io.IOException: Cannot run program "nonexistant" (in directory "/home/jodersky/p/buildsio"): error=2, No such file or directory
  at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1141)
  at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1072)
  at os.proc.proc$lzyINIT1$7(ProcessOps.scala:128)
  at os.proc.proc$1(ProcessOps.scala:133)
  at os.proc.spawn(ProcessOps.scala:135)
  at os.proc.call(ProcessOps.scala:76)
  ... 28 elided
Caused by: java.io.IOException: error=2, No such file or directory
  at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
  at java.base/java.lang.ProcessImpl.<init>(ProcessImpl.java:313)
  at java.base/java.lang.ProcessImpl.start(ProcessImpl.java:244)
  at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1108)
  ... 33 more

Li Haoyi

@lihaoyi

it's arguable whether the check flag should take effect when an executable@is not found; if people think it should we can change the logic

jodersky

@jodersky

What should the command result be in such a case?

if we set a synthetic exit code (such as 127), then we'd follow the semantics of a classic shell

Where communities thrive

People

Repo info

Activity