No, I have the Lstat() already modified and working. Now I created the “infinite recursion prevension” in (*fs.walkFilesystem).Walk(), but it doesn’t work because the conversion doesn’t work.
Edit: Or was your question meant as an answer?
The conversion doesn’t work because fs.FileInfo does not implement os.FileInfo (no Sys() interface{} method). To get the os.FileInfo something like info.(basicFileInfo).FileInfo would work, but you can’t use that in walkFs, as it must work with fs.Filesystem, not an implementation of that like fs.BasicFilesystem. If you need SameFile in walkFs, you’ll likely have to add that to the fs.Filesystem interface. Or more generally: You shouldn’t use os outside of BasicFilesystem, that package should be abstracted by filesystem.go.
From the end:
I can implement fs.SameFile() in BasicFilesystem, but this wouldn’t solve my problem. I would have the same conversion problem there, I suppose. In Walk() I use the conversion in a safe way, i.e. if the conversion cannot be done, I do perform a fallback behaviour.
Edit: info.(basicFileInfo).FileInfo can be compiled, sorry. Actually the conversion does something, thank you!
I need to log errors into the Syncthing log, in walkFilesystem. How should I do that?
The logger l that is available in the fs package has error/fatal/panic methods omitted. Should I add them to the logger.Logger interface? Why are they omitted?
The logger in fs uses the same interface as all the other packages, so it has (for example) Warnln and Warnf for logging errors to the user and the corresponding Info... methods for more informational things. That said, do we really want errors to the user from inside fs?
About the SameFile stuff, I suggest adding some sort of unique file identifier to our basicFileInfo that could be used for this purpose.
type basicFileInfo struct {
// ...
UniqueIdentifier interface{}
}
For example the inode in Unixes, or file ID in Windows. If we stored it as interface{} we could add a typed value from the actual implementation, such as a type unixInode int64 and then someInfo.UniqueIdentifier = unixInode(42) or in the Windows case ... = windowsFileIdentifier(1234). These values can be compared with == in fs.SameFile and that’ll only match if the type and value is identical. Also works with s3BucketHask("abcd1234") and such in the future.
No need then for gnarly type assertions in SameFile. And, possibly, no need for SameFile at all as it just compiles to an == and we could do that directly on the UniqueIdentifier instead.
I’ve got that already implemented and working. When I’ll send a pull request, you can review whether you want some changes to the code. I’m not completely satisfied with my solution, because I’m limited with my knowledge of Golang, and this requires more than a basic knowledge of Golang. But it works, and it is not ugly :-).
Actually, I need to use assertions. But it seems that there is no such thing in Golang. Hence, I want - at least - to log the situations when something doesn’t behave as expected. So that they are not silently ignored, they should - at least - occur in the log. Currently I use fmt.Printf().
1 Like
If you’re looking for something assertion-like, that’s if foo { panic("bug: foo happened") }. The cases where that’s warranted are few and far between
though, think “something happened that proves our assumptions are wildly incorrect and continuing at this point will eat the user’s data”.
For something of note that should end up in the log, use l.Infoln. If it’s something serious and actionable that the user can fix use l.Warnln.
Exactly. I think this is one of the places when if something gets wrong, data loss can occur.
1 Like
Keeping in mind that Syncthing will be dead in the water and the action will probably be retried repeatedly with the same crash. So if there’s a safer option, better take that.
l.Warnf() seems to have a negative side effect that is it displayed to the user every time the warning is issued. Is there a way to show it only once, or once a day, or so?
I have tested mine changes on Windows. How can I test that it works on Linux etc. too? Would you test it after I send the pull request? Or what are my options?
If you need that kind of logic it needs to be implemented manually. We have similar things in some places.
If you create a unit test it will be run on Windows/Linux/Mac by the builders.
How can I execute a particular unit test (e.g. walkfs_test.go)?
I need to create a Windows specific test. Would it work if I simply create walkfs_windows_test.go, or do I have to do something more?
Do I have to make cleanup, or the tests are executed with a clean filesystem? testWalkSkipSymlink() does no cleanup.
I’ll have to create a directory junction using system osexec.Command("cmd", "/c", "mklink", ...) or something like that.
Tests should clean up after themselves. (Maybe it doesn’t, but it should.) Keep in kind you can’t assume the tests run with admin privileges. It may not be feasible to automatically create junctions and test traversing them. There may be other aspects you can reasonably test.
Could you please advise me, how to run a unit test?
go test -run lib/fs/walkfs_test.go
doesn’t work.
go test -run=NameOfTestFunction but this has to be in a package directory.
You can omit the -run part and it will run for the whole package.
Alternatively, you can do go test github.com/syncthing/syncthing/lib/fs which is the import path to test.
Thank you, but I always get
can't load package: package .: build constraints exclude all Go files in <syncthing_src_root>
when run from the <syncthing_src_root>
Source root does not contain any source code, the code is either in cmd/something or lib/something.
You can just run ./build.sh test if you want to just run all tests.
I want to run that single test only, I do not know what running all the tests would do on my computer.
C:\Workspaces\Syncthing\lib>go test -run=fs/walkfs_test
can't load package: package .: no Go files in C:\Workspaces\Syncthing\lib
C:\Workspaces\Syncthing\lib>go test fs/walkfs_test
can't load package: package fs/walkfs_test is not in GOROOT (C:\Program Files\go\src\fs\walkfs_test)Run takes a function name, not a directory. The non-run argument takes a fully qualified (not relative) go package name (import path), not a directory.
C:\Workspaces\Syncthing\lib>go test -run=fs.testWalkSkipSymlink
can't load package: package .: no Go files in C:\Workspaces\Syncthing\lib
C:\Workspaces\Syncthing\lib>go test -run=fs/testWalkSkipSymlink
can't load package: package .: no Go files in C:\Workspaces\Syncthing\lib
C:\Workspaces\Syncthing\lib>go test -run=fs/walkfs_test.go/testWalkSkipSymlink
can't load package: package .: no Go files in C:\Workspaces\Syncthing\lib
I simply don’t know. Could you please write me down a command that works?