Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS: Handle deprecated stat64 #267

Closed
wants to merge 1 commit into from
Closed

Conversation

henryleberre
Copy link
Contributor

@henryleberre henryleberre commented Jul 22, 2022

macOS has recently deprecated the use of the stat64 structure defined in sys/stat.h. Thus, this pull request introduces a configure-time check for whether stat64 is present in both CMake and Autotools, that is used to pick whether to use stat64 or stat. On macOS, the stat structure may now refer to the 32 or 64 bit version of the structure. On most systems it should be 64 bit version by default but I added an ifdef in silo.c for macOS that requests to use its larger counterpart:

CMakeLists.txt:

check_symbol_exists(stat64 "sys/stat.h" HAVE_STAT64)
check_symbol_exists(stat "sys/stat.h" HAVE_STAT)

if (HAVE_STAT64)
	add_definitions(-DHAVE_STAT64)
endif()

if (HAVE_STAT)
	add_definitions(-DHAVE_STAT)
endif()

configure.ac

dnl
dnl macOS has deprecated the use of the stat64 structure defined in sys/stat.h.
dnl Thus, we check whether stat64 is present before considering to use it.
dnl On macOS, the stat structure may refer to the 32 or 64 bit version of the
dnl structure.
dnl

AC_CHECK_MEMBER([struct stat64.st_dev], [AC_DEFINE(HAVE_STAT64)], [], [[#include <sys/stat.h>]])
AC_CHECK_MEMBER([struct stat.st_dev],   [AC_DEFINE(HAVE_STAT)],   [], [[#include <sys/stat.h>]])

AC_DEFINE([HAVE_STAT64], [], [Whether struct stat64 is present in sys/stat.h])
AC_DEFINE([HAVE_STAT],   [], [Whether struct stat   is present in sys/stat.h])

silo.c:

#if HAVE_SYS_STAT_H
#if SIZEOF_OFF64_T > 4 && defined(__APPLE__)
#define _DARWIN_USE_64_BIT_INODE
#endif // SIZEOF_OFF64_T > 4 && defined(__APPLE__)
#include <sys/stat.h>
#endif // HAVE_SYS_TYPES_H

Uses of the stat structure now look like:

#if SIZEOF_OFF64_T > 4 && (defined(HAVE_STAT64) || !defined(HAVE_STAT))
    struct stat64 s;
#else
    struct stat s;
#endif

The || !defined(HAVE_STAT) section of ifdef handles the case where the build system being used hasn't generated any HAVE_STAT64 or HAVE_STAT preprocessor definitions. It is mostly there as a safety net.

I installed autoconf, automake, autoconf-archive, and libtool on Arch Linux to regenerate the configure scripts, but had to temporally create the SiloWindows folder and manually patch one line of the generated ./configure script, adding ;; in a case switch statement for gnu). This seems to be the result of a minor bug with AX_CC_MAXOPT, however, my experience with autotools is limited. I confirmed the code configures and compiles on macOS and on my Linux system.

@markcmiller86 markcmiller86 self-assigned this Oct 25, 2022
@markcmiller86 markcmiller86 self-requested a review October 25, 2022 15:50
@markcmiller86 markcmiller86 removed their assignment Oct 25, 2022
@markcmiller86
Copy link
Member

@henryleberre I guess I dropped the ball in reviewing this. Many apologies.

I think the real change here is to configure.ac and then a slew of updated auto-gen'd files. Is that correct?

Do you know for certain...Is Autotools 2.71 indeed a requirement that comes with this change?

@henryleberre
Copy link
Contributor Author

@markcmiller86 These issues should now be resolved! I believe I was able to regenerate the autotools system without introducing any major changes. I must have run autoupdate while attempting to get autotools to work while working on my original commit. The git diff is much shorter now. I made sure Silo built on Linux and on M1/macOS (where the issue this PR is resolving is present). The tests (e.i make check and cmake -DSILO_ENABLE_TESTS=ON) don't compile (even on LLNL/Silo @ master) so I didn't run the test suite.

@henryleberre
Copy link
Contributor Author

@markcmiller86 Did you get a chance to take a look?

@markcmiller86
Copy link
Member

@markcmiller86 Did you get a chance to take a look?

I have not and the only relectance is due to so many unnecessary files changing. But I have an idea for minimizing files changed that still gets this incorporated. I'll just regen with same tool versions used for what is already there.

Our autoconf stuff is on the way out anyways so this shouldn't be as big a deal as I am making it.

@markcmiller86
Copy link
Member

But I have an idea for minimizing files changed that still gets this incorporated.

My idea didn't work. I'll make one more attempt on a different platform with original tool set.

@markcmiller86
Copy link
Member

Replaced with #299

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants