Really try dylib when compiling for Apple platforms #181
Conversation
There was a problem hiding this comment.
Thanks a lot for your help!
Could you adjust the commit message of ac242af to provide details on why this has to happen, or why the current logic is wrong?
The line that was changed was introduced in 09b31b3 by the author of #84, after all.
Further, is 0fe0781 related to the fix or a performance optimization/refactor?
Thanks for elaborating - this helps me gain an understanding of what I consider export-knowledge as CI isn't testing this, there isn't much I could validate here otherwise.
Really fixes rust-lang#84. This is what rust-lang#85 intended, as far as I can tell, but the logic was such that it didn't work for cross-compiles from e.g. Linux (apple_to_apple would be false, leading to the build_zlib path being taken)
It fixes another issue, which is that the dylib is still not used when cross-compiling from Linux because the zlib_installed test fails as described in the commit message. |
There was a problem hiding this comment.
Thanks!
In the last moment before merging I found something that might be an issue, probably depending on what get_compiler() actually resolves to.
A quick check yielded this:
❯ cc src/smoke.c -o /dev/null -lz
src/smoke.c:4:10: warning: cast to smaller integer type 'int' from 'uLong (*)(uLong, const Bytef *, uInt)' (aka 'unsigned long (*)(unsigned long, const unsigned char *, unsigned int)') [-Wpointer-to-int-cast]
return (int) adler32;
^~~~~~~~~~~~~
1 warning generated.
libz-sys ( main) [$]
❯ cc src/smoke.c -o -g0 /dev/null -lz
src/smoke.c:4:10: warning: cast to smaller integer type 'int' from 'uLong (*)(uLong, const Bytef *, uInt)' (aka 'unsigned long (*)(unsigned long, const unsigned char *, unsigned int)') [-Wpointer-to-int-cast]
return (int) adler32;
^~~~~~~~~~~~~
1 warning generated.
clang: error: unable to execute command: Segmentation fault: 11
clang: error: linker command failed due to signal (use -v to see invocation)
| cmd.arg("src/smoke.c").arg("-o").arg("/dev/null").arg("-lz"); | ||
| cmd.arg("src/smoke.c") | ||
| .arg("-o") | ||
| .arg("-g0") |
There was a problem hiding this comment.
Are you sure this is working?
What I can gather from the CC documentation is the following:
-o <file>
Write output to file.
It seems like /dev/null wants to remain right behind it. Can you elaborate?
There was a problem hiding this comment.
Duh, that's obviously because I misplaced the argument.
There was a problem hiding this comment.
This would mean that this code probably never ran. Can you help me validate this, or share how you validated it?
There was a problem hiding this comment.
So, I did have the right patch originally, but I messed up something before I pushed, or something. It definitely worked. Anyways, here's how I checked: I added a panic at the beginning of build_zlib(). Then PATH=/path/to/cctools/bin:$PATH TARGET_CFLAGS="isysroot /path/to/MacOSX14.2.sdk" CC=clang cargo build --target aarch64-apple-darwin
There was a problem hiding this comment.
Thanks for your help.
That works for me, but it does so both on main and on this PR, after adding a panic right at the beginning of build_zlib().
This was my invocation (on MacOS): TARGET_CFLAGS="isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk" CC=clang cargo build --target aarch64-apple-darwin, while noting that my SDK folder is empty, probably not downloaded. I also assume that isysroot is a magic value.
My expectation would be that there is an invocation that panics on main to indicate it's picking up the bundled zlib, but won't panic here as proof that it will pickup the system library more reliably. For me this seems to happen no matter what.
Does it panic for you on main but not on the branch? Is PATH important for specific cctools? Is isysroot magic or am I supposed to replace it - are these flags making all the difference?
Thanks for bearing with me.
There was a problem hiding this comment.
Maybe on a Mac I wouldn't be able to reproduce it then anyway?
I do have a linux VM I could use, but would probably missing all the setup thats's needed to cross-compile.
Just to be clear, I can just merge this but after having found that one flaw I'd like to have proof that it's actually doing what it advertises. As the interested parties surely have a way to reproduce the issue they have with main the lack of issue with the code in this branch, maybe some logs could be shared to workaround difficulties in me reproducing it locally?
There was a problem hiding this comment.
Actually, if you add a panic to build_zlib, it will fail to cargo build on mac on main.
There was a problem hiding this comment.
That definitely doesn't happen for me :/. Here is my patch in case there is something I am missing.
diff --git a/build.rs b/build.rs
index 1368a12..2b11d3e 100644
--- a/build.rs
+++ b/build.rs
@@ -91,6 +91,7 @@ fn main() {
}
fn build_zlib(cfg: &mut cc::Build, target: &str) {
+ panic!("shouldn't be here");
let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap());
let lib = dst.join("lib");This is the slightly adjusted command-line I have been using: TARGET_CFLAGS="-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk" CC=clang cargo build --target aarch64-apple-darwin.
libz-sys ( main) +1 [$!]
❯ TARGET_CFLAGS="-isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk" CC=clang cargo build --target aarch64-apple-darwin
Compiling libc v0.2.152
Compiling pkg-config v0.3.29
Compiling cc v1.0.83
Compiling libz-sys v1.1.15 (/Users/byron/dev/github.com/rust-lang/libz-sys)
warning: unreachable statement
--> build.rs:95:5
|
94 | panic!("shouldn't be here");
| --------------------------- any code following this expression is unreachable
95 | let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
|
= note: `#[warn(unreachable_code)]` on by default
warning: unused variable: `cfg`
--> build.rs:93:15
|
93 | fn build_zlib(cfg: &mut cc::Build, target: &str) {
| ^^^ help: if this is intentional, prefix it with an underscore: `_cfg`
|
= note: `#[warn(unused_variables)]` on by default
warning: unused variable: `target`
--> build.rs:93:36
|
93 | fn build_zlib(cfg: &mut cc::Build, target: &str) {
| ^^^^^^ help: if this is intentional, prefix it with an underscore: `_target`
warning: `libz-sys` (build script) generated 3 warnings
Finished dev [unoptimized + debuginfo] target(s) in 1.16s
libz-sys ( main) +1 [$!]
There was a problem hiding this comment.
This is what I get on unpatched main:
% git clone https://github.com/rust-lang/libz-sys
Cloning into 'libz-sys'...
remote: Enumerating objects: 1377, done.
remote: Counting objects: 100% (419/419), done.
remote: Compressing objects: 100% (203/203), done.
remote: Total 1377 (delta 216), reused 380 (delta 200), pack-reused 958
Receiving objects: 100% (1377/1377), 902.13 KiB | 2.89 MiB/s, done.
Resolving deltas: 100% (656/656), done.
% cd libz-sys
% patch -p1
diff --git a/build.rs b/build.rs
index 1368a12..2b11d3e 100644
--- a/build.rs
+++ b/build.rs
@@ -91,6 +91,7 @@ fn main() {
}
fn build_zlib(cfg: &mut cc::Build, target: &str) {
+ panic!("shouldn't be here");
let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap());
let lib = dst.join("lib");
patching file build.rs
% cargo build
Updating crates.io index
Downloaded libc v0.2.153
Downloaded 1 crate (740.6 KB) in 1.00s
Compiling libc v0.2.153
Compiling pkg-config v0.3.29
Compiling vcpkg v0.2.15
Compiling cc v1.0.83
Compiling libz-sys v1.1.15 (/Users/glandium/libz-sys)
warning: unreachable statement
--> build.rs:95:5
|
94 | panic!("shouldn't be here");
| --------------------------- any code following this expression is unreachable
95 | let dst = PathBuf::from(env::var_os("OUT_DIR").unwrap());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
|
= note: `#[warn(unreachable_code)]` on by default
warning: unused variable: `cfg`
--> build.rs:93:15
|
93 | fn build_zlib(cfg: &mut cc::Build, target: &str) {
| ^^^ help: if this is intentional, prefix it with an underscore: `_cfg`
|
= note: `#[warn(unused_variables)]` on by default
warning: unused variable: `target`
--> build.rs:93:36
|
93 | fn build_zlib(cfg: &mut cc::Build, target: &str) {
| ^^^^^^ help: if this is intentional, prefix it with an underscore: `_target`
warning: `libz-sys` (build script) generated 3 warnings
error: failed to run custom build command for `libz-sys v1.1.15 (/Users/glandium/libz-sys)`
Caused by:
process didn't exit successfully: `/Users/glandium/libz-sys/target/debug/build/libz-sys-bdb7c6d6b951d63d/build-script-build` (exit status: 101)
--- stdout
cargo:rerun-if-env-changed=LIBZ_SYS_STATIC
cargo:rerun-if-changed=build.rs
cargo:rerun-if-env-changed=ZLIB_NO_PKG_CONFIG
cargo:rerun-if-env-changed=PKG_CONFIG_aarch64-apple-darwin
cargo:rerun-if-env-changed=PKG_CONFIG_aarch64_apple_darwin
cargo:rerun-if-env-changed=HOST_PKG_CONFIG
cargo:rerun-if-env-changed=PKG_CONFIG
cargo:rerun-if-env-changed=ZLIB_STATIC
cargo:rerun-if-env-changed=ZLIB_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_STATIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_aarch64-apple-darwin
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_aarch64_apple_darwin
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_aarch64-apple-darwin
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_aarch64_apple_darwin
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_aarch64-apple-darwin
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_aarch64_apple_darwin
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
cargo:rerun-if-env-changed=ZLIB_STATIC
cargo:rerun-if-env-changed=ZLIB_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_STATIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_aarch64-apple-darwin
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_aarch64_apple_darwin
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_aarch64-apple-darwin
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_aarch64_apple_darwin
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_aarch64-apple-darwin
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_aarch64_apple_darwin
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
cargo-warning=Could not run `PKG_CONFIG_ALLOW_SYSTEM_CFLAGS=1 pkg-config --libs --cflags zlib`
The pkg-config command could not be found.
Most likely, you need to install a pkg-config package for your OS.
Try `brew install pkg-config` if you have Homebrew.
If you've already installed it, ensure the pkg-config command is one of the
directories in the PATH environment variable.
If you did not expect this build to link to a pre-installed system library,
then check documentation of the libz-sys crate for an option to
build the library from source, or disable features or dependencies
that require pkg-config.
OPT_LEVEL = Some("0")
TARGET = Some("aarch64-apple-darwin")
HOST = Some("aarch64-apple-darwin")
cargo:rerun-if-env-changed=CC_aarch64-apple-darwin
CC_aarch64-apple-darwin = None
cargo:rerun-if-env-changed=CC_aarch64_apple_darwin
CC_aarch64_apple_darwin = None
cargo:rerun-if-env-changed=HOST_CC
HOST_CC = None
cargo:rerun-if-env-changed=CC
CC = None
cargo:rerun-if-env-changed=CRATE_CC_NO_DEFAULTS
CRATE_CC_NO_DEFAULTS = None
DEBUG = Some("true")
CARGO_CFG_TARGET_FEATURE = Some("aes,crc,dit,dotprod,dpb,dpb2,fcma,fhm,flagm,fp16,frintts,jsconv,lor,lse,neon,paca,pacg,pan,pmuv3,ras,rcpc,rcpc2,rdm,sb,sha2,sha3,ssbs,v8.1a,v8.2a,v8.3a,v8.4a,vh")
cargo:rerun-if-env-changed=CFLAGS_aarch64-apple-darwin
CFLAGS_aarch64-apple-darwin = None
cargo:rerun-if-env-changed=CFLAGS_aarch64_apple_darwin
CFLAGS_aarch64_apple_darwin = None
cargo:rerun-if-env-changed=HOST_CFLAGS
HOST_CFLAGS = None
cargo:rerun-if-env-changed=CFLAGS
CFLAGS = None
running "cc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-gdwarf-2" "-fno-omit-frame-pointer" "-arch" "arm64" "-Wall" "-Wextra" "src/smoke.c" "-o" "/dev/null" "-lz"
--- stderr
src/smoke.c:4:10: warning: cast to smaller integer type 'int' from 'uLong (*)(uLong, const Bytef *, uInt)' (aka 'unsigned long (*)(unsigned long, const unsigned char *, unsigned int)') [-Wpointer-to-int-cast]
return (int) adler32;
^~~~~~~~~~~~~
1 warning generated.
error: cannot parse the debug map for '/dev/null': The file was not recognized as a valid object file
clang: error: dsymutil command failed with exit code 1 (use -v to see invocation)
thread 'main' panicked at build.rs:94:5:
shouldn't be here
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
There was a problem hiding this comment.
It occurs to me that maybe you have pkg-config from brew?
When compiling for Apple platforms, with the output set to /dev/null, the test fails because clang invokes dsymutil on the output, and that fails on /dev/null. clang doesn't invoke dsymutil if compiling without debug info.
There was a problem hiding this comment.
This is the answer to this comment.
Thanks, that was it! Disabling pkg-config allowed me to reproduce the issue above on main, showing that it's already not working. On this branch, however, it does successfully compile.
I also believed to have validated that it does dynamically link due to a -l z flag when building the final build product as part of cargo build.
Thus this PR definitely appears to be an improvement that should be merged.
Thanks again for your patience in getting this PR over the finishing line - adjustments to the build system are among the most difficult to review.
Really fixes #84