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

unix: unbreak macOS < 10.14 #4230

Merged
merged 1 commit into from
Nov 18, 2023
Merged

Conversation

barracuda156
Copy link
Contributor

@bnoordhuis @johnsonjh Could we have this fixed, please?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I'm okay with merging this (sans style nit) but, per SUPPORTED_PLATFORMS.md, the breakage is completely intentional. If Apple doesn't support it, then we don't either.

This patch might buy you some time but we remove code for unsupported platforms when it gets in the way; sooner or later something is going to break irrevocably.

src/unix/fs.c Outdated Show resolved Hide resolved
@barracuda156
Copy link
Contributor Author

barracuda156 commented Nov 17, 2023

@bnoordhuis Thank you! Yes, I understand that nothing is forever, but this is a trivial and non-invasive fix, which won’t hurt (at least as long as the fallback code there is present).

UPD. Made a change pointed out by @kencu to rather use MAC_OS_X_VERSION_MIN_REQUIRED and squashed commits.

Co-authored-by: Ben Noordhuis <[email protected]>
@vtjnash
Copy link
Member

vtjnash commented Nov 18, 2023

Historically I don't think libuv has accepted patches for compile-time-based restrictions either, generally relying instead on runtime checks. For example, marking this a weak symbol (in the source and linker stages) and checking if it is null at runtime to decide which needs to be used.

@bnoordhuis
Copy link
Member

@vtjnash you're not wrong but for unsupported platforms I think it's fine to not sink too much time and effort into it. I'll go ahead and merge this.

@bnoordhuis bnoordhuis merged commit 4785ad6 into libuv:v1.x Nov 18, 2023
26 checks passed
@barracuda156
Copy link
Contributor Author

@bnoordhuis Thank you for merging!

@vtjnash My idea was to minimize needed changes, and since the fallback was already there, and these macros work reliably on macOS, there seemed to be no reason to re-implement it otherwise.

@@ -84,7 +84,8 @@

#if defined(__CYGWIN__) || \
(defined(__HAIKU__) && B_HAIKU_VERSION < B_HAIKU_VERSION_1_PRE_BETA_5) || \
(defined(__sun) && !defined(__illumos__))
(defined(__sun) && !defined(__illumos__)) || \
(defined(__APPLE__) && MAC_OS_X_VERSION_MIN_REQUIRED <= 101300)
Copy link
Contributor

@Bo98 Bo98 Nov 18, 2023

Choose a reason for hiding this comment

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

This doesn't seem correct:

ssize_t pwritev(int, const struct iovec *, int, off_t) __DARWIN_NOCANCEL(pwritev) __API_AVAILABLE(macos(11.0), ios(14.0), watchos(7.0), tvos(14.0));

It should be < 110000.

I can open a PR for that though. Have had a few people on 10.15 ask me about it downstream.

@vtjnash
Copy link
Member

vtjnash commented Nov 18, 2023

these macros work reliably on macOS

This isn't exactly true though, which is why we don't historically allow build time configurations to influence the runtime behavior. It only works for you because you aren't testing various combinations of new and old build vs. runtime machines.

@barracuda156
Copy link
Contributor Author

@vtjnash You mean, like, targeting 10.6 from 10.15 or alike? Indeed we do not normally do that, though presumably it should still work.

Availability macros are used across numerous software and come from Apple system headers. It is not something Macports invented. Perhaps I should have added that they work reliably in a standard environment. It may be possible to design a specific case to break them, but the whole issue was about fixing at least a standard basic usage.

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.

4 participants