alexcrichton / curl-rust

Rust bindings to libcurl
MIT License
1.02k stars 235 forks source link

Improve support for older macOS versions #314

Closed ohadravid closed 5 years ago

ohadravid commented 5 years ago

I tried running a binary built with the static-curl feature on macOS El Captian, and got the following error:

dyld: Symbol not found: _clock_gettime
  Referenced from: /..../my_binary (which was built for Mac OS X 10.13)
  Expected in: /usr/lib/libSystem.B.dylib

Apparently clock_gettime wasn't added until macOS 10.12. There is a similar issue for curl which says to not define HAVE_CLOCK_GETTIME_MONOTONIC, so I changed this in the build.rs and everything seems to be working fine.

alexcrichton commented 5 years ago

@sagebind given that this was added in https://github.com/alexcrichton/curl-rust/pull/305, do you have thoughts on this?

sagebind commented 5 years ago

Interesting. Well, it seems like the "right way" would be to target macOS 10.11 when you compile in order for the resulting binary to work on macOS 10.11. If you look at the code here, you can see that we use clock_gettime only if it is known to be available based on our compile target. In theory, this can be done like the following:

env CFLAGS=-mmacosx-version-min=10.11 cargo build

Indeed this probably ought to be the recommended approach if you want to produce a macOS executable that is compatible with versions older than your build system. Unfortunately this doesn't work for me right now because of #279, but you get the idea at least. Not defining HAVE_CLOCK_GETTIME_MONOTONIC gets us back into only having timing accuracy to the second.

Unless we define HAVE_MACH_ABSOLUTE_TIME. I shall go test that to see if it is viable.

ohadravid commented 5 years ago

@sagebind I tried it, but it still produces the bad symbol 😢

In my playground project's Cargo.toml:

curl = { features = ["static-curl"] , path = "/.../curl-rust" }

(The version in the path is like https://github.com/alexcrichton/curl-rust/pull/283 just to make it compile)

Running

$ cargo clean
$ env CFLAGS=-mmacosx-version-min=10.11 cargo build
...
Finished dev [unoptimized + debuginfo] target(s) in 13.31s
$ nm -g ./target/debug/playground | grep _clock
                 U _clock_gettime
sagebind commented 5 years ago

Well the symbol will still be there, but it is a weak symbol in that it will be used only if it is available on the current system. I would try running the binary you produced this way on a macOS 10.11 system. It should acknowledge that _clock_gettime is NULL and use an alternative implementation instead, but use _clock_gettime automatically on a new system.

alexcrichton commented 5 years ago

@ohadravid Can you try swapping this out for HAVE_MACH_ABSOLUTE_TIME? I think that's an OSX thing that should work?

sagebind commented 5 years ago

I did some digging and it looks like mach_absolute_time() is the right way to do performance timing before macOS 10.12, and it won't be deprecated any time soon, so I'm good with disabling HAVE_CLOCK_GETTIME_MONOTONIC altogether on macOS.

While setting an old macOS target version is still the "right way" to guarantee compatability with old versions, its not worth the trouble in this situation when HAVE_MACH_ABSOLUTE_TIME works perfectly fine on all versions going back to at least 2005. I tested this locally today (albeit on a new macOS version) and it works well. I'd recommend this approach:

     } else {
         if target.contains("-apple-") {
-            cfg.define("__APPLE__", None).define("macintosh", None);
+            cfg.define("__APPLE__", None)
+                .define("macintosh", None)
+                .define("HAVE_MACH_ABSOLUTE_TIME", None);
+        } else {
+            cfg.define("HAVE_CLOCK_GETTIME_MONOTONIC", None)
+                .define("HAVE_GETTIMEOFDAY", None);
         }

         cfg.define("RECV_TYPE_ARG1", "int")
-            .define("HAVE_CLOCK_GETTIME_MONOTONIC", None)
-            .define("HAVE_GETTIMEOFDAY", None)
             .define("HAVE_PTHREAD_H", None)
             .define("HAVE_ARPA_INET_H", None)
             .define("HAVE_ERRNO_H", None)
ohadravid commented 5 years ago

@sagebind you're totally right about the weak symbol, I didn't know this was a thing and indeed the -mmacosx-version-min=10.11 flag works.

I applied your suggested fix and I can confirm it runs on my El Capitian test machine! Thanks a lot!

alexcrichton commented 5 years ago

👍