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

mc: remove build workaround for macOS 10.13 (High Sierra) #24224

Merged
merged 2 commits into from
Jun 1, 2024

Conversation

zyv
Copy link
Contributor

@zyv zyv commented May 31, 2024

Description

This is the port of Homebrew/homebrew-core#173306 to MacPorts. I now know thanks to Homebrew CI that it at least works on all still supported macOS versions. Original description follows.


We introduced support for nanosecond timestamps with utimensat years ago and only enabled it if OS supported it.

This "worked" fine util macOS 10.13 High Sierra, when Apple introduced utimensat in a half-assed POSIX-incompatible way (apparently the name of the the st structure members were like st_atimespec instead of st_atim etc.). So the function was there, but our code couldn't use it and builds failed.

We neither had macOS, nor the kernel was published, so we couldn't fix it and the workaround here was to disable utimensat by force even if it was available.

I have now checked the builds on the latest version of macOS and they work, and also the nanosecond timestamps work, because apparently Apple fixed the POSIX compatibility somewhen along the way.

The current workaround is now causing data loss (file timestamps are always truncated to microseconds) for the users.

The problem is, I don't know when Apple fixed this. If the formula builds on Monterey and Ventura, then it will also work, but I don't have these systems. High Sierra is not supported for years.

Can we just remove this workaround?

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 12 / 13 / 14

@jmroot
Copy link
Member

jmroot commented May 31, 2024

It doesn't look like the way struct stat is defined ever changed. Whether you get the old or new one is still controlled by _POSIX_C_SOURCE and _DARWIN_C_SOURCE in the current SDK: https://github.com/apple-oss-distributions/xnu/blob/main/bsd/sys/stat.h#L109

@zyv
Copy link
Contributor Author

zyv commented May 31, 2024

It doesn't look like the way struct stat is defined ever changed.

Ha-ha, the comment sounds very inspiring:

XXX So deprecated, it would make your head spin

If I understand it correctly now, nothing has really changed at the OS X side, st_atimespec is the new one and what you get per default. The legacy solution with extra members (st_atime & st_atimensec) is non-default and you get it if you set _POSIX_C_SOURCE.

So you are right, the plot thickens, I have found the following in our code:

/* struct stat members */
#ifdef __APPLE__
#define st_atim st_atimespec
#define st_ctim st_ctimespec
#define st_mtim st_mtimespec
#endif

I cannot yet reconstruct what exactly has changed in our tree since the workaround of disabling utimensat was added to your code. Apparently too many people did weird changes without really understanding what they are doing, but the end result at the moment seems to be that it somehow works now and your workaround can be removed.

So I stand by my PR :) but I will have to read the POSIX standard though to understand what's the right way to use utimensat und how it was before. Our mess has to be cleaned up. I'm pretty sure if one understands what one's doing, switching between utime and utimensat can be implemented cleanly with structure member checks that would work on all systems.

@jmroot
Copy link
Member

jmroot commented May 31, 2024

Yes, I agree with removing the workaround, I just don't think it will necessarily break any older systems.

@@ -37,10 +37,6 @@ patchfiles patch-src_subshell_common.c.diff

configure.args --without-x --enable-vfs-sftp=no

if {[vercmp ${macos_version} 10.13] >= 0} {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're removing this workaround, I think we will still need to increment revision for affected systems in order rebuild without and update the port archive:

if {[vercmp ${macos_version} 10.13] >= 0} {
    revision [expr ${revision}+1]
}

@jmroot can confirm if this is needed.

@zyv
Copy link
Contributor Author

zyv commented Jun 1, 2024

In the next release, we'll hopefully support all stat structure formats - past, present and future ;) Thank you folks for the tips!

MidnightCommander/mc@25b2cc4

@jmroot jmroot merged commit 5cd74a6 into macports:master Jun 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants