Replace boost.filesystem #207

C0nsultant · 2018-05-27T20:19:11Z

Create a minimally slim replacement for boost.filesystem for WIN32 and
POSIX compliant systems.

Closes #70.

Create a minimally slim replacement for boost.filesystem for WIN32 and POSIX compliant systems.

ax3l · 2018-05-28T07:39:26Z

CMakeLists.txt

@@ -209,6 +210,24 @@ set(IO_SOURCE
        src/IO/HDF5/HDF5IOHandler.cpp
        src/IO/HDF5/ParallelHDF5IOHandler.cpp)

+if(WIN32)


I think we can just do #if defined(_WIN32) in-code and assume all others are POSIX.
Ref: https://stackoverflow.com/questions/5919996/how-to-detect-reliably-mac-os-x-ios-linux-windows-in-c-preprocessor

Also, I think it's really unsave to set those vars globally and publicly, e.g. in the tests below ;-)

I will push a commit to fix this...

ax3l · 2018-05-28T07:42:50Z

include/openPMD/auxiliary/Filesystem.hpp

+list_directory(std::string const& path );
+
+void
+create_directories( std::string const& path );


docstring: this is mkdir -p, right?

That was the intention, to save the user from creating loops or copy-pasting.

ax3l · 2018-05-28T07:43:26Z

include/openPMD/auxiliary/Filesystem.hpp

+create_directories( std::string const& path );
+
+void
+remove_file( std::string const& path );


docstring: this is equivalent to rm with -r or -f or both or none?

As this will be implemented with remove(), at least -r.
-f should probably not be the default with void - the user should be notified if the target does not exist. We could borrow from std::filesystem::remove(), return bool and then make -f the default.

ax3l · 2018-05-28T07:43:53Z

include/openPMD/auxiliary/Filesystem.hpp

+file_exists( std::string const& path );
+
+std::vector< std::string >
+list_directory(std::string const& path );


docstring: non-recursive, files-only?

Non-recursive, every entry (i.e. directories and files).

Makes sense. Best to just put this info in the doxygen doc string I guess.

ax3l · 2018-05-28T07:44:36Z

include/openPMD/auxiliary/Filesystem.hpp

+namespace auxiliary
+{
+bool
+path_exists( std::string const& path );


is this directory_exists?

Yes, it actually checks for a directory. This should be made more explicit.

ax3l · 2018-05-28T07:45:20Z

test/AuxiliaryTest.cpp

+    REQUIRE(path_exists("/home"));
+    REQUIRE(!path_exists("/nonexistent_folder_in_root_directory"));
+#endif
+}


missing end-of-file newline

We support either windows or POSIX systems. Keep the checks of defines local for it.

ax3l · 2018-05-29T11:55:18Z

Nice, FindHDF5.cmake seems to forget to expose it needs to link against zlib. Fun.

Knhihihi.

C0nsultant · 2018-05-29T12:08:09Z

Do not be mistaken:

Unknown compression format zlib. This might mean that compression will not be enabled.

is a message by our own code:

openPMD-api/src/Dataset.cpp

Lines 68 to 70 in b8e8de9

    
           std::cerr << "Unknown compression format " << format 
        
                     << ". This might mean that compression will not be enabled." 
        
                     << std::endl;

The current failures are probably because mkdir -p is not yet working, but mkdir is (there's only a test for the latter).

ax3l · 2018-05-29T12:13:13Z

Ah ok, I thought it's triggered the other way around. Alright, you can pull again and work on it.

ax3l · 2018-05-29T15:24:09Z

@C0nsultant I think you should be able to traverse your Unix paths for nested creation of dirs like this:

#include <iostream>
#include <sstream>

void iterEm(std::string input)
{
    std::istringstream ss(input);
    std::string token;

    std::string iterPath;
    if( input.find("/") == 0 )
        iterPath += "/";
    while( std::getline( ss, token, '/' ) ) {
        if( token.size() > 0 )
            iterPath += token + "/";
        std::cout << iterPath << std::endl;
    }
}

int main()
{
    iterEm( "/abc/def/ghi" );
    iterEm( "abc/def/ghi" );
    iterEm( "abc/def/ghi/" );

}

output:

/
/abc/
/abc/def/
/abc/def/ghi/

abc/
abc/def/
abc/def/ghi/

abc/
abc/def/
abc/def/ghi/

Recursively delete directories (like `rm -r`). Recursively create directories (like `mkdir -p`).

ax3l

I am confused about deletes in cmake dirs ;)

ax3l · 2018-05-29T20:43:31Z

test/AuxiliaryTest.cpp

+    {
+        std::string del = "..\\CMakeFiles\\" + *dir_entries.begin();
+        while( !file_exists(del) && directory_exists(del) )
+        {


:-o what are you removing here? xD

ax3l · 2018-05-29T20:43:31Z

test/AuxiliaryTest.cpp

+            dir_entries = list_directory(del);
+            del += "/" + *dir_entries.begin();
+        }
+        if( file_exists(del) )


This looks scary and probably harmful, e.g. for make install.

C0nsultant · 2018-05-30T03:53:02Z

The idea was to find a file to test deletion on realiably. Maybe we could delete some .cpp files instead?

Use CI support script directory rather than CMake build directory

ax3l · 2018-05-30T05:42:15Z

Nooo, don't let us mess with the cake build tree our even worse the source tree :-o Just use c++ standard file IO to write a new text file for deletion.

ax3l · 2018-05-30T14:57:47Z

I will push an update on this in the next hour.

Create a simple file and remove it again. Avoids touching the CMake tree and source tree.

ax3l · 2018-05-30T15:24:32Z

src/auxiliary/Filesystem.cpp

+
+    bool success = true;
+#ifdef _WIN32
+    success = CreateDirectory(path.c_str(), NULL);


recursive?
https://stackoverflow.com/a/1530815/2719194

(or same as the solution below for POSIX)

That or
https://msdn.microsoft.com/en-us/library/bb762130(v=vs.85).aspx

This function creates a file system folder whose fully qualified path is given by pszPath. If one or more of the intermediate folders do not exist, it creates them.
To set security attributes on a new folder, use SHCreateDirectoryEx.

C0nsultant · 2018-05-30T15:28:15Z

src/auxiliary/Filesystem.cpp

+        throw std::system_error(std::error_code(errno, std::system_category()));
+    dirent* entry;
+    while ((entry = readdir(directory)) != nullptr)
+        if( strncmp(entry->d_name, ".", 1) && strncmp(entry->d_name, "..", 2) )


This probably skips hidden folders starting with a . we could need checks for both strncmp if the strlen is larger than 1 or 2 respectively.

Recursively delete directories (like `rm -r`). Recursively create directories (like `mkdir -p`).

ax3l · 2018-05-31T07:18:09Z

src/auxiliary/Filesystem.cpp

+    auto mk = [](std::string const& p) -> bool { return CreateDirectory(p.c_str(), nullptr); };
+#else
+    char seperator = '/';
+    auto mk = [](std::string const& p) -> bool { return (0 == mkdir(p.c_str(), 0777));};


Maybe let us take the default, environment umask instead of 0777?
https://stackoverflow.com/questions/10147990/how-to-create-directory-with-right-permissons-using-c-on-posix

This would actually make Windows and POSIX behaviour equivalent. Agreed!

ax3l · 2018-05-31T07:20:56Z

test/AuxiliaryTest.cpp

+    auto entries = list_directory(".");
+    for( auto const& entry : entries )
+        std::cout << entry << std::endl;
+    //REQUIRE(file_exists("AuxiliaryTests.exe"));


should these be enabled or removed?

should the output above be on or is it a debug leftover?

These should be enabled, they just failed because I could not figure out the cwd in appveyor.
Also, the above std::cout loop should be removed again.

For appveyor, this should then be

REQUIRE(file_exists("Release\\AuxiliaryTests.exe"));

Yes, depends if it's a Debug or Release build ("Configuration") in multi-config build systems such as VS that is used as Generator. Maybe just do not check for this exe and check for another file?

Any file whose (relative) path we can reliably determine is fine, really.
Going from appveyor logs, the cwd is C:\projects\openpmd-api\build\bin.
How about

REQUIRE(file_exists("..\\..\\CMakeLists.txt"));

Yep, this is just by the WORKING_DIRECTORY I add to the add_test calls in CMakeLists.txt. I am surprised it is not bin\Release\ ...

Update: Actually REQUIRE(file_exists("AuxiliaryTests.exe")); should work as is - can you try again?

Checking the CMakeLists.txt this way is not possible, since it depends on the user-chosen location of the build directory. Let me see which file we can use...

If Python bindings are built, it appears we could check for

REQUIRE(file_exists(".\\2_read_serial.py"));

~~Thinking about it, why not just do the same empty file workaround from 0b87334?~~
Actually, I think we are good with just relying on 0b87334, which does a file_exists() check on a relative path.
This can be removed, after all.

C0nsultant · 2018-05-31T10:11:55Z

Once both of these minor issues are addressed, we should be ready to merge. 🎆

ax3l · 2018-05-31T10:47:40Z

Yay, nice! 🚀 Do you want to push it or shall I do it?

C0nsultant · 2018-05-31T11:52:52Z

@ax3l Pull the trigger.

ax3l · 2018-05-31T12:12:57Z

We are free, freeee!

Begin to replace boost.filesystem

fb9e57d

Create a minimally slim replacement for boost.filesystem for WIN32 and POSIX compliant systems.

C0nsultant added third party third party libraries that are shipped and/or linked refactoring Boost internal backend labels May 27, 2018

C0nsultant self-assigned this May 27, 2018

C0nsultant added this to To do in First Stable Release via automation May 27, 2018

C0nsultant requested a review from ax3l May 27, 2018 20:19

C0nsultant moved this from To do to In progress in First Stable Release May 28, 2018

ax3l reviewed May 28, 2018

View reviewed changes

C0nsultant and others added 4 commits May 28, 2018 21:45

Replace all required fs operations

312745f

Windows fs test cases

fd1caba

Fix missing cstring include

99a1581

FS Platform: Local

ed84f20

We support either windows or POSIX systems. Keep the checks of defines local for it.

ax3l force-pushed the topic/replace_boost.filesystem branch from c32fa93 to ed84f20 Compare May 29, 2018 11:15

Remove all traces of Boost

411228e

Knhihihi.

ax3l force-pushed the topic/replace_boost.filesystem branch from 8bf6353 to 411228e Compare May 29, 2018 12:12

Handle nested paths in POSIX

2847752

Recursively delete directories (like `rm -r`). Recursively create directories (like `mkdir -p`).

ax3l reviewed May 29, 2018

View reviewed changes

Change file deletion test target

1d3b688

Use CI support script directory rather than CMake build directory

ax3l mentioned this pull request May 30, 2018

Versioning Scheme #87

Open

Auxiliary Test: Create & Remove File

0b87334

Create a simple file and remove it again. Avoids touching the CMake tree and source tree.

ax3l reviewed May 30, 2018

View reviewed changes

C0nsultant commented May 30, 2018

View reviewed changes

C0nsultant added 2 commits May 30, 2018 20:20

Handle nested paths in Windows

5ec719f

Recursively delete directories (like `rm -r`). Recursively create directories (like `mkdir -p`).

Remove shadowing name, fix Windows

82c4646

ax3l changed the title ~~[WIP] Replace boost.filesystem~~ Replace boost.filesystem May 31, 2018

ax3l reviewed May 31, 2018

View reviewed changes

C0nsultant added 2 commits May 31, 2018 13:17

Remove redundant Windows unit tests

a7884cb

Respect umask permissions in POSIX

126514a

ax3l approved these changes May 31, 2018

View reviewed changes

ax3l merged commit 12b8abd into openPMD:dev May 31, 2018

First Stable Release automation moved this from In progress to Done May 31, 2018

C0nsultant deleted the topic/replace_boost.filesystem branch May 31, 2018 12:14

C0nsultant mentioned this pull request Jun 11, 2018

First Python Numpy Bindings #219

Merged

Replace boost.filesystem #207

Replace boost.filesystem #207

Conversation

C0nsultant commented May 27, 2018 • edited

Choose a reason for hiding this comment

ax3l May 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant May 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ax3l commented May 29, 2018

C0nsultant commented May 29, 2018 • edited

ax3l commented May 29, 2018

ax3l commented May 29, 2018 • edited

ax3l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant commented May 30, 2018 • edited

ax3l commented May 30, 2018 via email

ax3l commented May 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant May 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant May 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant May 31, 2018 • edited

Choose a reason for hiding this comment

ax3l May 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant May 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0nsultant commented May 31, 2018

ax3l commented May 31, 2018 • edited

C0nsultant commented May 31, 2018

ax3l commented May 31, 2018

C0nsultant commented May 27, 2018 •

edited

ax3l May 29, 2018 •

edited

C0nsultant May 28, 2018 •

edited

C0nsultant commented May 29, 2018 •

edited

ax3l commented May 29, 2018 •

edited

C0nsultant commented May 30, 2018 •

edited

ax3l commented May 30, 2018 •

edited

C0nsultant May 30, 2018 •

edited

C0nsultant May 31, 2018 •

edited

C0nsultant May 31, 2018 •

edited

ax3l May 31, 2018 •

edited

C0nsultant May 31, 2018 •

edited

ax3l commented May 31, 2018 •

edited