Fix Quick Start #527
Fix Quick Start #527
Conversation
src/storage/FileLoader.cpp
Outdated
| } | ||
|
|
||
| for (auto& dirElement : std::filesystem::directory_iterator(dir)) { | ||
| removedItemsCount += std::filesystem::remove_all(dirElement.path()); |
There was a problem hiding this comment.
It might be worth to be defensive here, and log and ignore any exceptions if e.g. the file permissions don't let you remove something, rather than crashing the program
There was a problem hiding this comment.
Yes, thanks for pointing this out, I have added some more logging/exception handling around the remove_all call.
Just to double-check, I understand you suggest logging the error and throw an exception?
There was a problem hiding this comment.
I think not throwing an exception would be fine here, because not being able to remove stale files doesn't necessarily have to stop the entire program. It's a common workflow with some utilities to take away write permissions from files you want to make sure are not modified/removed by a program, usually when debugging issues. But this is just my personal preference, there are equally valid reasons to crash here - to avoid inconsistent state that only shows up in logs that might not be read.
| boost::filesystem::path objPath(conf.objectFileDir); | ||
| objPath.append(directory); | ||
| std::filesystem::path objPath(conf.objectFileDir); | ||
| objPath += directory; |
There was a problem hiding this comment.
path.append works slightly different in std::filesystem. Namely if appending two paths both of which start with a backslash, then the first one will be ignored, and the second one taken by an absolute path (unless otherwise flagged in the std::filesystem::path object).
There was a problem hiding this comment.
OK sure, this seems like odd behaviour to me (and clearly to the boost::filesystem authors too), so could you put a comment above this as a warning? I also think you mean forward slash rather than backslash (unless you're running on windows 😄)
76cbc45 to
255eace
Compare
255eace to
f8eddb1
Compare
| gen.codegenForSharedObject(localSharedObjFile); | ||
|
|
||
| // Check the locally cached path matches the expected one | ||
| REQUIRE(loader.getSharedObjectObjectFile(localSharedObjFile) == objFile); |
There was a problem hiding this comment.
With the new path.append behaviour in std::filesystem this check would have failed.
This is, before:
path.append("/tmp/obj", "/usr/local/faasm/foo/bar.o"); // = "/usr/local/faasm/foo/bar.o")Now:
"/tmp/obj" += "/usr/local/faasm/foo/bar.o"; // = "/tmp/obj/usr/local/faasm/foo/bar.o"| REQUIRE(!boost::filesystem::exists(sharedPath)); | ||
|
|
||
| // Check the shared files dir exists, but not the specific shared file path | ||
| REQUIRE(std::filesystem::exists(conf.sharedFilesDir)); |
There was a problem hiding this comment.
Without the changes in FileLoader::clearLocalCache() this chech would fail.
Before, remove_all(conf.sharedFilesDir) would delete conf.sharedFilesDir as well.
Now, we use removeAllInside that recursively deletes the contents of conf.sharedFilesDir but not the directory itself.
Shillaker
left a comment
There was a problem hiding this comment.
Couple of style changes but other than that looks good 👍
| boost::filesystem::path objPath(conf.objectFileDir); | ||
| objPath.append(directory); | ||
| std::filesystem::path objPath(conf.objectFileDir); | ||
| objPath += directory; |
There was a problem hiding this comment.
OK sure, this seems like odd behaviour to me (and clearly to the boost::filesystem authors too), so could you put a comment above this as a warning? I also think you mean forward slash rather than backslash (unless you're running on windows 😄)
This PR fixes three problems with the filesystem interaction after flush:
boost::filesystem::remove_all(dir)woudl also remove the directory it was called on. Albeit not necessarily an error, I am pretty sure this was not intended. Instead I add a helper method to remove all contents inside a directory.boost::filesystem::create_directories(path)was throwing an uncaught exception complaining about "no such path or directory" as a consequence of 1.std::filesystem::path::appendbehaves differently: if two absolute paths are appended the first one is ignored.For both problems I add a check in the tests that would fail without this PR. Additionally, I bump the cpp dependency to fix another problem with the python task flushing the workers.
I also change the
boost::filesystemcalls tostd::filesystemonly in the files I change, not globally. Happy to re-factor everything now or in a separate PR.Lastly, bump the version code after the last commit to make sure the quick start test actually picks up the latest code changes. This does not completely fix the problem until we have real per-commit e2e testing.