Make project compile with clang 11.0.1-2 in Mac

[johnfea]

Description

I needed these changes for the project to compile with clang 11.0.1-2 with osx sdk 10.14.

It would probably need some kind of detection for missing utimensat() support and then this code as a fallback for this to be accepted. Not sure why project maintainers have no compile issues related to the other parts.

I can keep applying this patch locally but its probably a good idea to try to make the project compile with more compilers.

Tests

All changes are trivial except for src/libutil/filesystem.cpp which calls utime() instead of utimensat() to change file last modification date on Mac.

Checklist:

• I have read the contribution guidelines.
• If this is more extensive than a small change to existing code, I
  have previously submitted a Contributor License Agreement
  (individual, and if there is any way my
  employers might think my programming belongs to them, then also
  corporate).
• I have updated the documentation, if applicable.
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• My code follows the prevailing code style of this project.

[lgritz]

That's a bit of an awkward construction. Let's reverse the sense of this, so it's

#ifdef _WIN32
   ... windows case ...
#elif defined(__APPLE__)
   ... apple case ...
#else
    ... default unix case ...
#endif

I'm confused about why this is necessary. utimensat is POSIX, should be fine on Apple, and indeed on the Mac laptop I'm using this very moment, "man utimensat" shows me the right thing.

On the other hand, utime() is also POSIX, also exists on Linux, and is simpler, so I'm wondering if maybe the simpler solution is not to split apple from linux, and just use your new utime call for both? That is, maybe this is the best construct:

void
Filesystem::last_write_time(string_view path, std::time_t time) noexcept
{
#ifdef _WIN32
    struct _utimbuf times;
    times.actime  = time;
    times.modtime = time;
    _wutime(u8path(path).c_str(), &times);
#else
    struct utimbuf times;
    times.actime  = time;
    times.modtime = time;
    utime(u8path(path).c_str(), &times);
#endif
}

will that work for both Apple and Linux just fine?

[lgritz]

Please omit this change. We don't alter formatting or whitespace in code unrelated to the subject of a PR.

[lgritz]

This change is fine, I guess, but I don't understand why it is necessary. What error message did you get with the old code?

[lgritz]

Is this stock clang 11 or "Apple clang" 11? I'm confused about the reason why you're getting errors and the rest of us aren't. Can you paste in the exact error messages you get?

[johnfea]

/src/libutil/filesystem.cpp:786:5: error: 'utimensat' is only available on macOS 10.13 or newer
Maybe this is because I compile with: "-mmacosx-version-min=10.9".
Your version of that patch looks good to me unless there's some functionality in the newer function that is missed.
EDIT: utime() has only one second precision. Its probably a good idea to retain using the newer function with fallback to utime() or better utimes() which has microsecond precision for build target older than 10.13.

/src/libOpenImageIO/imagebuf.cpp:680:11: error: use of overloaded operator '+=' is ambiguous (with operand types 'std::string' (aka 'basic_string<char, char_traits, allocator>') and 'OpenImageIO_v2_5_0::string_view' (aka 'basic_string_view'))
This clang is not allowing this even though string_view has operator to convert to std::string.

The changes in src/include/OpenImageIO/strutil.h weren't required after all to build which is good since that was the worst part of this patch.

Also, what I missed in this patch was that src/cmake/compiler.cmake this section:
if (NOT ${PROJECT_NAME}_SUPPORTED_RELEASE OR DEFINED ENV{${PROJECT_NAME}_CI})
[..]
adds "-Werror" which makes /src/libOpenImageIO/imagebufalgo_draw.cpp not compile.
Not sure what would be the correct patch for this or if its an issue with my cmake command.

As to why only I'm getting these errors its very likely because I have cross compiling setup that builds for three different operating systems with a single build command on one Linux machine. It saves a lot of time compared to booting Mac and Windows each time to build my project and I use it also to build all dependencies that compile with it like oiio. This clang is probably different version or setup little different.

Not sure if I can modify this pull request or if I have to create a new one.

[lgritz]

You can just push another commit to that same branch, and it will automatically update the PR here.

[lgritz]

I think just using utime where available is fine. There's no real reason to use utimensat as far as I can tell.

[johnfea]

After a second look, utimensat() code inputted 0 for nanoseconds so utime() will probably give identical result.

I've updated the patch. This code compiles both in Linux and Mac. Adding cmake option: -DOpenImageIO_SUPPORTED_RELEASE=1 fixed the -Werror issue.

[lgritz]

I don't know, I think the part about

auto r = strtoull(std::string(s).c_str(), nullptr, 10);

might still be a change you might want to propose, if you ever need to build from master with full warnings.

The way we use OpenImageIO_SUPPORTED_RELEASE=1 is that it's turned off for master or when doing CI tests, so we get -Werr -- which may occasionally break the build when compilers upgrade and add new warnings, or as in your case, if a user tries to build with a compiler version different from what we tested on, and a new warning is encountered. We turn the option on (=1) for the release branch, so that actual users will be less prone to have their build break if they encounter warnings we haven't seen before.

The basic philosophy is that OIIO developers themselves want every warning to stop the build so it forces us to fix the warnings, but "users" building the software on their end should just get a completed build and not have an unanticipated warning make it so they can't build the software. Generally speaking, those people needing a bullet-proof build probably also want a fully stable official release, so they're not working in the master branch.

So your call, I guess -- whether you want to add in those fixes so you don't get the warnings ever, or if you want to leave that code alone and remember to do the SUPPORTED_RELEASE on your end.

[johnfea]

OpenImageIO_SUPPORTED_RELEASE=1 only affected compilation of /src/libOpenImageIO/imagebufalgo_draw.cpp which isn't part of my patch.

"auto r = strtoull(std::string(s).c_str(), nullptr, 10);" compiles now without errors even with -Wall -Werror. I made the initial patch in 2022, maybe something has changed since then e.g. I might have a different clang version now or maybe it just was never required.

Even with SUPPORTED_RELEASE=1, I still need all parts of the second patch for the code to compile currently. So my current take is to propose pull request for the second patch.