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

libuv-devel: update to 1.50.0 #27671

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

casr
Copy link
Contributor

@casr casr commented Feb 15, 2025

Description

update libuv-devel to 1.50.0

This is a follow up to #27670 but restricted to only libuv-devel upon the suggestion of @barracuda156 (#27670 (comment))

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

macOS 14.7.1 23H222 arm64
Xcode 15.4 15F31d

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL in commit message?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@herbygillot for port libuv.
@michaelld for port libuv.

@macportsbot macportsbot added type: update maintainer: open Affects an openmaintainer port labels Feb 15, 2025
@barracuda156
Copy link
Contributor

@casr To a little surprise, libuv upstream introduced more breakages, so it won’t build on a number of macOS versions (unrelated to architecture), but I will give you a patch to fix their amazing code.

@barracuda156
Copy link
Contributor

barracuda156 commented Feb 15, 2025

@casr So there are two more issues now which need to be fixed:

  1. libuv/libuv@c0a61c3 commit has broken mach time.
  2. libuv/libuv@1c778bd commit uses non-existing on legacy OS symbols.

Without a fix for these, there are two failures: 1) undefined _mach_continuous_time at linking, 2) that _recvmsg_x/_sendmsg_x stuff fails to compile, since not supported in the SDK.

Here is a patch draft which fixes the build (basically reverting the first breakage and disabling unsupported stuff). Notice, I do not know exact macOS version threshold for these. It is safe to say the patch is required at least on 10.6, but probably some laters OSs too.
In this form it should not be applied unconditionally (but it can be easily amended to that end).

patch-libuv-fix-1.50.0.txt

@barracuda156
Copy link
Contributor

@macportsraf @fhgwright If you got time (and interest), please have a look, maybe you could fix these two issues properly.

Prefer a regex replace over patch as Makefile.in is dynamically
generated so this can ease maintenance burden.
@casr
Copy link
Contributor Author

casr commented Feb 16, 2025

Excellent sleuthing and thank you for the patches. I'll add them in now.

  1. I was considering if the maintenance of these patches might be helped if they were broken up into logical units that we could then save/apply with git format-patch and git am. It's more pleasant to rebase them after you have it applied to properly to an older working version after all. [Edit: see b28f0fc but can be backed out if it's not desirable]
  2. Considering that libuv's support policy for MacOS >= 11, I wonder if might be prudent to guard the patches for older OS releases with a block like:
    platform darwin {
        # versions prior to MacOS 11
        if { ${os.version} < 20 } {
            patchfiles-append 0002-x.patch \
                              0003-y.patch \
                              0004-z.patch
        }
    }
    The downside being that these patches may drift from continuing to be applied unless someone is testing on these older OSs. The upside being that we do not introduce any subtle issues on supported OSs with our meddling. I wonder if the downside can be helped by introducing a comment about testing by modifying the guard to be something like if { true || ${os.version} < 20 } { (or perhaps MacPorts has an easier method for this?)

What are your thoughts on these points?

patchfiles-append patch-no-libutil-on-Tiger.diff
post-patch {
# Tiger has no libutil
reinplace -E "s|(.*DARWIN_TRUE.*-lutil.*)|# \\1|" Makefile.in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@barracuda156 Do you know if this is sufficient as compared with the patch? The patch originally commented out DragonFlyBSD's, FreeBSD's, etc as well which seemed unnecessary for Tiger to work.

If it is necessary then the regex could instead be:

Suggested change
reinplace -E "s|(.*DARWIN_TRUE.*-lutil.*)|# \\1|" Makefile.in
reinplace -E "s|(.*-lutil.*)|# \\1|" Makefile.in

which would yield a similar result to the patch.

(Extra context: I changed this to be a reinplace instead because this file is dynamically created by automake so will more likely apply in the future)

Convert patch set to be compatible with `git am`. Set produced with
`git format-patch`
@casr casr force-pushed the libuv-1.50.0-devel branch from 0320657 to b28f0fc Compare February 16, 2025 21:55
@barracuda156
Copy link
Contributor

The downside being that these patches may drift from continuing to be applied unless someone is testing on these older OSs. The upside being that we do not introduce any subtle issues on supported OSs with our meddling. I wonder if the downside can be helped by introducing a comment about testing by modifying the guard to be something like if { true || ${os.version} < 20 } { (or perhaps MacPorts has an easier method for this?)

What are your thoughts on these points?

@casr Perhaps this is up to this port’s maintainers to decide. Abstractly, it is always better to use unconditionally applied patches with appropriate conditions in the code itself (partly because in practice too often conditional patches are not rebased and then fail to apply). However if maintainers have no interest in dealing with patches, it is comparatively better to have patches which may fail to apply until someone bothers to fix that, rather than support for older systems being removed and patches going out of the window.

@fhgwright
Copy link
Contributor

@barracuda156

@casr So there are two more issues now which need to be fixed:

  1. libuv/libuv@c0a61c3 commit has broken mach time.

That code had issues even before the change:

  1. It wasn't cacheing the dlsym() result. That means it was performing the lookup on every call, probably slowing it down by a couple of orders of magnitude.
  2. The version of the scaling calculation it was using is preferable from an accuracy standpoint, but it fails miserably on PPC, due to overflow. The result becomes garbage after less than 7.5 minutes of uptime.

It would be even simpler than their "simplified" version to just use clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW), which exists natively on 10.12+, and is available from legacy-support in older OS versions (as of v1.4.0). The timescale in the latter case would be based on mach_absolute_time() rather than mach_continuous_time() (as was the old code), but the only difference is in the behavior while sleeping.

  1. libuv/libuv@1c778bd commit uses non-existing on legacy OS symbols.

I don't know what the difference is between recvmsg_x/sendmsg_x and the normal recvmsg/sendmsg, but one can easily see that the former pair appeared as syscalls in 10.10 but have never had function wrappers in any OS version.

@casr

  1. I was considering if the maintenance of these patches might be helped if they were broken up into logical units that we could then save/apply with git format-patch and git am. It's more pleasant to rebase them after you have it applied to properly to an older working version after all.

Patchfiles are always a terrible way to maintain non-trivial changes, from both readability and maintainability perspectives. What I normally do in such cases is to create a branch in my fork of the upstream code (in some cases more than one branch), and make all changes as git commits in the fork. Then, git diff (preferably with a bit of postprocessing) can be used to generate a MacPorts patchfile. For new upstream releases, I start by merging the new version into the modification branch(es), resolve the conflicts, make any needed additional changes, and generate (a) new patchfile(s). There's no reason to have per-topic patchfiles when they're not the primary representation of the changes.

@barracuda156
Copy link
Contributor

@fhgwright Could you help us with the fix for the time issue here? Would be nice to have it working correctly, including powerpc, since a number of ports rely on libuv.

@fhgwright
Copy link
Contributor

@barracuda156

@fhgwright Could you help us with the fix for the time issue here? Would be nice to have it working correctly, including powerpc, since a number of ports rely on libuv.

As I previously suggested:

diff --git a/src/unix/darwin.c b/src/unix/darwin.c
index 009efbef..251545d8 100644
--- a/src/unix/darwin.c
+++ b/src/unix/darwin.c
@@ -25,8 +25,7 @@
 #include <stdint.h>
 #include <errno.h>
 
-#include <mach/mach.h>
-#include <mach/mach_time.h>
+#include <time.h>
 #include <mach-o/dyld.h> /* _NSGetExecutablePath */
 #include <sys/resource.h>
 #include <sys/sysctl.h>
@@ -51,15 +50,9 @@ void uv__platform_loop_delete(uv_loop_t* loop) {
 }
 
 
-static void uv__hrtime_init_once(void) {
-  if (KERN_SUCCESS != mach_timebase_info(&timebase))
-    abort();
-}
-
-
 uint64_t uv__hrtime(uv_clocktype_t type) {
-  uv_once(&once, uv__hrtime_init_once);
-  return mach_continuous_time() * timebase.numer / timebase.denom;
+  (void) type;
+  return clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW);
 }
 
 

Although they're technically correct about clock_gettime_nsec_np() calling mach_timebase_info() on every call, the latter has been internally cacheing the timebase parameters since 10.12, so the only cost (on any OS they support) is the miniscule cost of one function call, which is hardly a good reason to go against Apple's recommendation to use clock_gettime_nsec_np() instead of mach_continuous_time(). And in fact, their uv_once() mechanism is almost certainly more expensive than the one function call in Apple's code, so their version is probably slower as well as being more complicated than the straightforward approach (which also happens to work on older systems via legacy-support).

And in general, they tend to use CLOCK_MONOTONIC instead of CLOCK_MONOTONIC_RAW, even though the latter is more reliably accurate on most systems.

casr added 2 commits February 20, 2025 22:29
Add in Fred Wright's patch
Restrict recvmsg_x/sendmsg_x to 10.10+
@casr
Copy link
Contributor Author

casr commented Feb 21, 2025

Thank you for the patch. I've replaced the reverted version with this new one. 6ae2d1e

  1. libuv/libuv@1c778bd commit uses non-existing on legacy OS symbols.

I don't know what the difference is between recvmsg_x/sendmsg_x and the normal recvmsg/sendmsg, but one can easily see that the former pair appeared as syscalls in 10.10 but have never had function wrappers in any OS version.

Given this, I've adjust the patch relating to this code to need at least 10.10. 584f34b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: update
Development

Successfully merging this pull request may close these issues.

6 participants