Remove dependency on boost filesystem

[meshula]

Description of Change(s)

This change introduces an implementation of MkDir to Arch that functions similarly to boost::filesystem's mkdir - it recursively builds a path as necessary. A small test is included to demonstrate functionality.

Fixes Issue(s)

boost::filesystem is used only once, trivially, by the USD suite in the Houdini plug in to create a hierarchy of directories. It seems that retaining this boost dependency for such a simple function is unnecessary.

Linking against boost libraries in general poses continuing challenges especially in tracking compatibility between compiler and cmake versions. Removing this solitary dependency on boost::filesystem in favor of a function in Arch brings us one step closer to having only boost::python as a link dependency.

[marktucker]

If boost::filesystem is only used to do a recursive mkdir, and that is only done from within the Houdini plugin, I'd suggest getting rid of the Arch implementation of the recursive mkdir, and using UT_FileUtil::makeDirs() from within the Houdini HDK instead. No need to pollute USD with code already implemented in Houdini...

[spitzak]

This sort of function is needed often enough that maybe it should be provided. I am greatly in favor of avoiding boost file functions due to their messing Unicode transformations into the filename object, which should be byte strings only.

This can be much shorter, just mkdir the string and check the error result. Only if it returns ENOENT do you recurse and try to create the parent directory. Other errors and success return ok.

[jtran56]

Filed as internal issue #USD-4881.

[meshula]

Updated the PR.

boost::system is also unused, so I've elided it.

I extended the unit test to try to catch more expected behaviors of when creating a directory should or should not succeed.

@spitzak I attempted a mkdir/ENOENT variation as you suggested, but haven't found a cross platform pattern that passes the extended unit test. This simple stat method does pass though.

I added for windows only a preprocessor definition that suppresses a pragma lib in boost's headers from bringing in spurious boost link libraries. This one is particularly vexing because the boost name mangling that pragma does frequently mismatches boost libraries end users are attempting to link with.

I added parens to a print in the copy python script to allow that script to run if your build environment has python3 in the path.

[spitzak]

What error do you get on Windows if the stat fails? I would add it to the set. I would be interested in what the error is as this sort of code is everywhere. Also to make sure, what I was proposing is this:

Error myMkdir(x) {
   error = mkdir(x);
   if (error == ENOENT) {
       error = myMkdir(parent(x));
       if (not error)
           error = mkdir(x);
   }
   return error;
}

I think adding whatever Windows fails with to the error test would fix this.

The main deal is to get the most likely course to do as few system calls as possible.

[meshula]

Yes, that's roughly the pattern I attempted. If _mkdir returns -1, you should be able to return ENOENT from errno. I didn't observe that to always be true, and the simple loop was becoming spaghetti... Pretty much it was a case that seemed only resolvable by calling stat, which basically took me all the way around the bush to this implementation :) But I'm happy to revisit it; I can dig a bit and find out where it failed.

[spitzak]

I see no reason why it should be spagetti. Maybe post an example of what you tried. All I can guess is that Windows is perverse and mkdir is returning success for a failure where making the parent directory works? Basically what I want is: int result = mkdir(this); bool mkdir_parent_might_work = some_kind_of_test(result, errno); if (mkdir_parent_might_work) { recursive call with parentl mkdir(this); } No matter how complex or perverse the some_kind_of_test(result,errno) is, I see no reason for it to ever be "spagetti". Maybe the confusion is that the result is "mkdir_parent_MIGHT_work", ie a false positive is ok. The main reason for this is so that the common case is a single call. I see this sort of overhead all the time and it really should never be put into common reusable functions. More practically it is so the returned errno is from mkdir and not from stat.

…

On Fri, Nov 2, 2018 at 6:44 PM Nick Porcino ***@***.***> wrote: Yes, that's roughly the pattern I attempted. If _mkdir returns -1, you should be able to return ENOENT from errno. I didn't observe that to always be true, and the simple loop was becoming spaghetti... Pretty much it was a case that seemed only resolvable by calling stat, which basically took me all the way around the bush to this implementation :) But I'm happy to revisit it; I can dig a bit and find out where it failed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#679 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABLBEUmWcgqBb8A5_JMcTtcZd56AuhIWks5urPT-gaJpZM4YBRKj> .

[spitzak]

Actually I think the problem is that you want this to report success if the directory already exists. The usage I was copying mine from did not call in this case (because it was not called until in fact the attempt to create a file in that directory failed). If the assumption is that the directory likely already exists, and you want it to fail if there is a non-directory file there, the stat seems the fastest. At least on Linux there seems to be no way to tell if EEXIST is due to the existing file being a directory or not. You should not need stat for any recursive calls but it is a bit of a pain to write it that way (the recursive function has to be a different one).

…

On Sun, Nov 4, 2018 at 1:53 PM Bill Spitzak ***@***.***> wrote: I see no reason why it should be spagetti. Maybe post an example of what you tried. All I can guess is that Windows is perverse and mkdir is returning success for a failure where making the parent directory works? Basically what I want is: int result = mkdir(this); bool mkdir_parent_might_work = some_kind_of_test(result, errno); if (mkdir_parent_might_work) { recursive call with parentl mkdir(this); } No matter how complex or perverse the some_kind_of_test(result,errno) is, I see no reason for it to ever be "spagetti". Maybe the confusion is that the result is "mkdir_parent_MIGHT_work", ie a false positive is ok. The main reason for this is so that the common case is a single call. I see this sort of overhead all the time and it really should never be put into common reusable functions. More practically it is so the returned errno is from mkdir and not from stat. On Fri, Nov 2, 2018 at 6:44 PM Nick Porcino ***@***.***> wrote: > Yes, that's roughly the pattern I attempted. If _mkdir returns -1, you > should be able to return ENOENT from errno. I didn't observe that to always > be true, and the simple loop was becoming spaghetti... Pretty much it was a > case that seemed only resolvable by calling stat, which basically took me > all the way around the bush to this implementation :) But I'm happy to > revisit it; I can dig a bit and find out where it failed. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#679 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABLBEUmWcgqBb8A5_JMcTtcZd56AuhIWks5urPT-gaJpZM4YBRKj> > . >

[meshula]

One possible simplifying semantic would be to return true indicating that a directory is created, and false indicating not. But then, the user will have to follow up with a stat to know if the desired directory exists and is a directory. Nothing's gained in terms of total system calls to accomplish the task. I prefer that the true indicates that a writeable directory exists/was made at the desired place, and false indicates that a writeable directory does not exist there. With my preference, naming the routine ArchMkDir gives a false equivalence to mkdir and _mkdir. Another possibility is to not call the routine ArchMkDir, or to go with the suggestion of not having the routine and using the Houdini routine instead. Writing a POSIX simulator is a slippery slope, as the faulty _mkdir shows.

[spitzak]

Apple's source showing how "mkdir -p" is implemented, I think this is based on the BSD code. It appears to use stat() like you are. It has an additional annoyance in that the umask for the parent directories seems to be different than for the final one: https://opensource.apple.com/source/file_cmds/file_cmds-45/mkdir/mkdir.c At least for the "final" directory it appears that stat is needed, you pointed out the same thing I did, which is that you want "a directory is already there" to not be an error, but "a file is there" to be an error.

…

On Mon, Nov 5, 2018 at 12:30 PM Nick Porcino ***@***.***> wrote: One possible simplifying semantic would be to return true indicating that a directory is created, and false indicating not. But then, the user will have to follow up with a stat to know if the desired directory exists and is a directory. Nothing's gained in terms of total system calls to accomplish the task. I prefer that the true indicates that a writeable directory exists/was made at the desired place, and false indicates that a writeable directory does not exist there. With my preference, naming the routine ArchMkDir gives a false equivalence to mkdir and _mkdir. Another possibility is to not call the routine ArchMkDir, or to go with the suggestion of not having the routine and using the Houdini routine instead. Writing a POSIX simulator is a slippery slope, as the faulty _mkdir shows. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#679 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABLBEcFfKYEgzP__l6b5HDsQYZENTbUEks5usJ_mgaJpZM4YBRKj> .

[c64kernal]

BTW, we've got a change coming to dev after the pending release that removes the boost::filesystem dependency in gusd and uses the Houdini UT method as @marktucker suggested.

[c64kernal]

@meshula -- I've also verified that in build_usd.py we need to keep boost::filesystem around if you're building with OIIO -- I'll add a conditional for that one as well. I hope it's okay with you, but I'll be merging in this change piecemeal with the end goal of getting rid of the dependency on boost::filesystem and boost::system if you're not building with OIIO...

[meshula]

Sounds good. I'm trying to reproduce this issue. Which platform(s) require boost::filesystem when using OIIO? Also, this is when using the build_usd.py script, I assume?

[c64kernal]

As far as I know, all platforms require boost::filesystem for OIIO, but I've been testing on Windows. And yeah this is with build_usd.py

I've landed several changes already that incorporate some of this PR. They should hit dev soon hopefully so that you can have a look.

Right now I'm working on removing more dependencies from build_usd.py and making them conditional on whether or not you build OIIO (we'll also have to test with the various plugins, e.g., Katana, Maya, etc.) and on the different platforms, but so far I haven't gotten to any of that -- I expect that we'll be able to do much better than we do today if you don't build all those plugins.

[meshula]

One more follow up - you've got BOOST_ALL_NO_LIB defined, when building the OIIO plugin for usd?

if(WIN32)
    # On Windows, boost automagic library linking via pragma lib is problematic, so suppress it
    add_definitions(-DBOOST_ALL_NO_LIB)
endif()

[c64kernal]

I don't have that set! Though I get the same issue on linux, and the missing dependency is expressed in cmake (somewhere!) before it hits the boost build system, I think.

FWIW, what I've got so far (still testing) is build filesystem only when OIIO is requested and build date_time and thread only if either OIIO or Katana plugins are requested. And again, this logic only lives in build_usd.py.

[c64kernal]

Hey @meshula -- the changes for this landed in dev. Have a look at them and let me know what you think. If you see something important that's missing, feel free to reopen or submit a new PR. Thanks so much!

[meshula]

Fantastic! I look forward to playing with it!