YosysHQ / sby

SymbiYosys (sby) -- Front-end for Yosys-based formal verification flows
Other
379 stars 74 forks source link

-f clean: QoL improvement on Windows concerning file/dir removal locking #232

Closed dlmiles closed 1 year ago

dlmiles commented 1 year ago

When using the -f argument be more forgiving with the expectation of a clean workspace and the expectation of the new sby run being responsible for directory creation.

This is a usability and quality of life improvement for Windows users where the OS can implement file and directory locking implicitly. In the EDA world it is common to have multiple tools in use at any one time and it can become tortious to have to close files / exit 3rd party applications to release locking so sby is happy to rerun.

This change will prevent sby claiming a terminal error has occurred when it fails to create a directory that already exists. It also now considers the environment to be 'clean' (as per -f) if all the non-directory elements of the file tree have been deleted, leaving potentially an empty a skeleton of directories.

dlmiles commented 1 year ago

If the original behaviour is a requirement, maybe it can be implemented as: -ff (where it errors like before at a directory already existing, when it goes to create it) and -fff (where it errors at the first moment a delete, create file or mkdir not being successful), because I assume at the moment the file/directory deletion code ignores error returns.

jix commented 1 year ago

I'm not super familiar with how windows behaves here, but when a file is locked, wouldn't the directory still have that file in it after attempting to recursively delete the directory and not be empty in that case? Is it common to have locked directories without locked files? Otherwise I don't quite see how this solves the issue when a file is locked.

Does windows allow you to rename a directory that contains locked files or locked directories? If that's the case, SBY could fall back to renaming the directory if it can't delete it, which would work even if some contained files can't be deleted.

dlmiles commented 1 year ago

The easiest test for you to probably observe is to make a sby report run. Open a cmd.exe, change directory into foobar_bmc. Then try to run sby again.

For me using gtkwave on Windows from a Desktop icon launch, if you open the VCD to display it, then run 'sby' to regenerate report again, then sby fails itself, the only file or directory left around is the foobar_bmc folder itself that is empty that gtkwave still has the previous VCD up on screen in.

I don't believe gtkwave keeps an open file handle to the file. Not sure on what gtkwave does for current working directory. Not sure if there is an MSYS2 (which gtkwave on windows is running from) environment interaction matter. Other tools that generate VCD can regenerate underneath gtkwave in the same situation, but sby just fails itself on an unimportant detail (when all the dirs are empty).

Re Windows API behaviour:

Generally speaking with standard Windows API calls using default options, an open file handle on a file, will prevent it from being unlink, renamed. I believe it will prevent the directory it is contained in from being renamed as well, but I'm not 100% sure on this one myself. In effect everything is pinned.

I believe there are options to allow Linux like behaviour for files, but it relies on the system operation caller to use these options everytime they open a file handle and that just does not happen, most application use default behaviour and legacy WIN32 API.

As for how directories themselves are handled when compared to Linux you'd need to experiment.

What is the purpose of falling back to renaming a directory ? foobar_bmc -> foobar_bmc_12345.deleteme just so it can be the one to mkdir foobar_bmc ? Linux will be fine it with, Windows not so much. You should be able to test this easily using cmd.exe and the 'cd' command. But I don't expect it to change the outcome of the problem being reported here. I don't think gtkwave will care if the old dir is renamed, I expect when the 'reload' button is pressed to look at the new VCD file it will use an absolute path to lookup the new file.

The solution at the moment is to close gtkwave (which might have multiple traces open, as you diagnose the coverage issue and test) so that the process exit. Then 'sby' will regenerate the data.

jix commented 1 year ago

I don't use Windows, so I can't easily test any of this, that's why I am asking. The change is simple enough that I don't need to reproduce this myself before merging this, but in that case I do want to understand the underlying issue and understand how this fixes it. The detail I was missing (that you mentioned in your answer) was that the current working directory of a program like gtkwave can end up having SBY's workdir (or a subdirectory of it) as its current working directory, implicitly locking it without having an explicit lock on any contained file.