Skip to content

Incorrect condition for st_mtim field name #1510

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

Closed
michaelforney opened this issue Dec 14, 2018 · 2 comments
Closed

Incorrect condition for st_mtim field name #1510

michaelforney opened this issue Dec 14, 2018 · 2 comments
Labels

Comments

@michaelforney
Copy link

I noticed an issue with the ifdefs in d2bf82f (nanosecond mtime fields). That commit seems to be misinterpreting how feature test macros work[0]. _POSIX_C_SOURCE, _XOPEN_SOURCE, _BSD_SOURCE and _SVID_SOURCE are defined by the application to tell the libc headers what to expose. They are not defined by the libc as the commit message says.

The POSIX standardized field name is st_mtim[1], so that should be used as the default and worked around only for platforms that don't have this field (i.e. macOS and apparently AIX). This will prevent having to extend the condition for every platform that uses the POSIX name (i.e. Solaris in 1952afa, and FreeBSD in febd3b3). This also breaks on musl, and I suspect BSDs as well.

[0] https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_node/libc_13.html
[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html

@markand
Copy link

markand commented Mar 20, 2019

Hello,

Is there any chance that the fix gets merged? It looks like Alpine also use this patch.

@jhasse
Copy link
Collaborator

jhasse commented Mar 20, 2019

Yes, someone needs to check if this doesn't break anything for AIX, Android, and Solaris and comment on #1513. In general I would prefer if we could test more of this with CI, see #1526.

@jhasse jhasse closed this as completed in 567815d Aug 6, 2019
jhasse added a commit that referenced this issue Aug 6, 2019
Use st_mtim if st_mtime is macro, fix #1510
skyleaworlder pushed a commit to AOSPworking/ninja-hacked that referenced this issue Jan 7, 2022
In POSIX.1-2008, sys_stat has a st_mtim member and a st_mtime backward
compatibility macro. Should help avoid hardcoding platform detection.

Bug: 190084016
Test: m out/soong/host/linux-x86/bin/ninja with musl
(cherry picked from commit 567815d)
Change-Id: If073a90b26d7dd4f3149ed76acc0da817bf07775
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants