[PATCH] D54883: [Support/FileSystem] Use st_atimespec only when __APPLE__ is defined

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D54883#1308380 <https://reviews.llvm.org/D54883#1308380>, @krytarowski wrote:

> In D54883#1308351 <https://reviews.llvm.org/D54883#1308351>, @MaskRay wrote:
>
> > In D54883#1308333 <https://reviews.llvm.org/D54883#1308333>, @krytarowski wrote:
> >
> > > In D54883#1308313 <https://reviews.llvm.org/D54883#1308313>, @MaskRay wrote:
> > >
> > > > In D54883#1307857 <https://reviews.llvm.org/D54883#1307857>, @aaron.ballman wrote:
> > > >
> > > > > In D54883#1307853 <https://reviews.llvm.org/D54883#1307853>, @krytarowski wrote:
> > > > >
> > > > > > In D54883#1307787 <https://reviews.llvm.org/D54883#1307787>, @aaron.ballman wrote:
> > > > > >
> > > > > > > Do our minimum system requirements for building LLVM include requiring POSIX 2008 conformance (perhaps indirectly, through the minimum toolchain requirements)?
> > > > > >
> > > > > >
> > > > > > There are almost no OSes having conformance with UNIX/POSIX. UNIXv6 (or similar) is Solaris only.
> > > > >
> > > > >
> > > > > If our existing system requirements aren't sufficient to practically guarantee these fields are available, I'd prefer to keep the autodetect code and rely on that instead.
> > > >
> > > >
> > > > There are 2 pieces of autodetect code:
> > > >
> > > > 1. preprocessor level detection `#if defined(HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC)`
> > > > 2. cmake detection `CHECK_STRUCT_HAS_MEMBER`
> > > >
> > > >   After this patch, they get simplified and become:
> > > > 3. preprocessor level detection `#ifdef __APPLE__`
> > > >
> > > >   On https://github.com/apple/darwin-xnu/blob/master/bsd/sys/stat.h , I cannot find a better feature macro to test so I have to use `__APPLE__`. When they start to support `st_atim` (which is widely available on other platforms) in a newer version, we can make the `#ifdef __APPLE__` check more specific.
> > > >
> > > >   The patch directly tests for the necessary prerequisite and removes two cmake variables. This makes it substantially more build system agnostic (for people who use gn,bazel,... they don't need to re-invent the feature detection mechanism in their build systems) and probably (ironically) more portable.
> > >
> > >
> > > Do you mean that someone reinvented build scripts in bazel? If so, it sounds like a downstream problem not relevant to the LLVM codebase. I consider this new version as inferior as every __OS__ check shall be ideally replaced with a check against feature. For example mentioned Illumos - SmartOS has a lot of APIs from Linux in order to host Linux-KVM and this is different to other Illumos distributions (and different to Solaris).
> >
> >
> > Have you read the description? illumos `usr/src/uts/common/sys/stat.h` has had `st_atim` for probably 14 years. `typedef struct timespec timestruc_t;`
> >
> > We are here discussing this specific feature of POSIX.1-2008, not overall POSIX.1-2008.
> >
> > > For example mentioned Illumos - SmartOS has a lot of APIs from Linux in order to host Linux-KVM and this is different to other Illumos distributions (and different to Solaris).
> >
> > Of course, but they are irrelevant here as we don't use these SmartOS/Linux specific features.
>
>
> This is an example of a mentioned OS in description when feature checks are plain wrong by design (even if this particular example might work for all illumos distributions). Another example, Minix keeps importing a lot of code from NetBSD, I don't intend to hardcode __minix__ here or there and keep iterating over 3rd party projects for every new feature in the OS.


Sorry if I didn't specify the NetBSD date when the `st_atim` support is added in the first revision, but I think i've added that before you made your first comment. Do you think a minix that is modern enough and the rest of the community should care about doesn't support `st_atim`? If it doesn't, I can extend the `#ifdef __APPLE__` check to include it. I have a feeling that some of our discussion may have diverged from the original purpose of this revision. It tries to simplify the various cmake feature detection and preprocessor detection. If the minix community cares if someone else may inadvertently break their builds, they should contribute a buildbot, otherwise, it is up to luck that nobody would break theirs. We should also not emphasize more on hypothetical platforms that don't have good POSIX.1-2008 support without a clear proof. At the least, for the platforms that are not widely used, it is not rare that people inadvertently break them but usually they can be fixed soon.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54883/new/

https://reviews.llvm.org/D54883