search

inflation / jpegxl-rs

GNU General Public License v3.0
56 stars 10 forks source link

Vendored build seems to fail on Windows and Mac OS #51

Open veluca93 opened 4 months ago

veluca93 commented 4 months ago

Context: I am adding JPEG XL support to Zola (https://www.getzola.org/), and I am getting build failures on Windows and Mac OS.

https://dev.azure.com/getzola/zola/_build/results?buildId=3265&view=logs&j=930b9528-a8f9-5f10-1483-94bc7df0e022&t=8a52e34c-7660-5ca7-92f5-cbd642549100

https://dev.azure.com/getzola/zola/_build/results?buildId=3265&view=logs&j=d2980ad8-b87d-5294-4faf-0af1145b0486&t=4895aa0f-ffeb-520a-bc7b-cf61e25f8fa6

I am afraid my knowledge of system dependencies in Rust is basically 0, so I don't have a very good idea of where to start to fix those.

I'm willing to help with upstream libjxl fixes if those were necessary/useful though :)

inflation commented 4 months ago

The Windows one is that you are linking with a different MSVC runtime library. By default, it will use MultiThreadedDLL (CMake) or /MD (link.exe) variant, which should be consistent with Rust itself. I'm not sure how zola build itself, maybe it's using a +crt-static setup for static linking. You could try to overwrite CMAKE_MSVC_RUNTIME_LIBRARY env for something else.

inflation commented 4 months ago

The macOS one is unfortunately a limitation of Rust. You need to manually set RUSTFLAGS=-C default-linker-libraries env somehow. The best way is probably a line in the doc for now.

veluca93 commented 4 months ago

The Windows one is that you are linking with a different MSVC runtime library. By default, it will use MultiThreadedDLL (CMake) or /MD (link.exe) variant, which should be consistent with Rust itself. I'm not sure how zola build itself, maybe it's using a +crt-static setup for static linking. You could try to overwrite CMAKE_MSVC_RUNTIME_LIBRARY env for something else.

Excuse my ignorance here :-) isn't defining that variable something that the build script should do? I do think Zola is producing a static build (haven't confirmed it though)

inflation commented 4 months ago

Excuse my ignorance here :-) isn't defining that variable something that the build script should do? I do think Zola is producing a static build (haven't confirmed it though)

Yes, but you would want to keep up with what Rust does by default: using release CRT and dynamic linking. It's somewhat of a "least surprising" principle.

veluca93 commented 4 months ago

Right, but couldn't the build script figure out that crt-static is being use and then set the variable conditionally?

On Sun, 5 May 2024, 11:45 Inflation, @.***> wrote:

Excuse my ignorance here :-) isn't defining that variable something that the build script should do? I do think Zola is producing a static build (haven't confirmed it though)

Yes, but you would want to keep up with what Rust does by default: using release CRT and dynamic linking. It's somewhat of a "least surprising" principle.

— Reply to this email directly, view it on GitHub https://github.com/inflation/jpegxl-rs/issues/51#issuecomment-2094714753, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOPAIZRM3MGAB7E6WMRDGLZAX5VNAVCNFSM6AAAAABHGZQSGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUG4YTINZVGM . You are receiving this because you authored the thread.Message ID: @.***>

inflation commented 4 months ago

In theory, yes. However, these customizations should left untouched so people can easily overwrite anything they need.

veluca93 commented 4 months ago

In theory, yes. However, these customizations should left untouched so people can easily overwrite anything they need.

From what I can see the env variable is set here: https://github.com/inflation/jpegxl-rs/blob/78bd163950c15e50e340f1afa8b566ecd38a2713/jpegxl-src/src/lib.rs#L42

Wouldn't adding another cfg option depending on crt-static just provide a better default and still allow people to overwrite what they might need to?

inflation commented 4 months ago

I don't think so.

veluca93 commented 4 months ago

I managed to find the configuration of the zola pipeline, and I don't see anything that would indicate that they are using crt-static on windows: https://github.com/getzola/zola/blob/b93640851eab8db6cc0f47419e050df2c762f3c4/azure-pipelines.yml#L39, so I would guess that the problem lies elsewhere.

While investigating this issue, I ran into https://github.com/rust-lang/cmake-rs - is there any reason not to use that crate?

inflation commented 4 months ago

While investigating this issue, I ran into https://github.com/rust-lang/cmake-rs - is there any reason not to use that crate?

Already using in jpegxl-src.

veluca93 commented 4 months ago

While investigating this issue, I ran into https://github.com/rust-lang/cmake-rs - is there any reason not to use that crate?

Already using in jpegxl-src.

Ah, sorry, I didn't see that - I was confused by the fact that cmake-rs seems to have logic to handle /MD already, and from your first message I understand that to have a similar purpose to passing CMAKE_MSVC_RUNTIME_LIBRARY.

I do not quite understand why it is being explicitly set here for Windows builds then - is the behaviour of cmake-rs incorrect?

inflation commented 4 months ago

I do not quite understand why it is being explicitly set here for Windows builds then - is the behaviour of cmake-rs incorrect?

Rust itself will always dynamic link against the multithreaded release version of CRT no matter what profile you are building. CMake will choose the CRT based on debug or release and static or dynamic.

inflation commented 4 months ago

After some investigation, it seems that the underlying cc crate should set those variables automatically, but that's not the case here. I'm going to discuss more with them.

inflation commented 3 months ago

53 should fix the Windows build issue for now.

I still haven't found a better solution for macOS. The problem is that libjxl needs something from compiler_rt but rustc passes -nodefaultlibs unconditionally. RUSTFLAGS=-C default-linker-libraries disable this option, but I couldn't find a way to let the crate carry this flag rather than a local Cargo config.

veluca93 commented 3 months ago

53 should fix the Windows build issue for now.

Great news!

I still haven't found a better solution for macOS. The problem is that libjxl needs something from compiler_rt but rustc passes -nodefaultlibs unconditionally. RUSTFLAGS=-C default-linker-libraries disable this option, but I couldn't find a way to let the crate carry this flag rather than a local Cargo config.

What does libjxl need from compiler_rt?

inflation commented 3 months ago

___cpu_models as shown in the error log. It's for CPU feature detection, which is why it's only happened on x86 (and why I didn't catch that)

veluca93 commented 3 months ago

I see, going from the error log I assume the issues to be cause by the calls to __builtin_cpu_supports in here https://github.com/libjxl/libjxl/blob/main/lib/jxl/enc_fast_lossless.cc#L4176 - would you be able to test if compilation works if you remove them?

inflation commented 3 months ago

I see, going from the error log I assume the issues to be cause by the calls to __builtin_cpu_supports in here https://github.com/libjxl/libjxl/blob/main/lib/jxl/enc_fast_lossless.cc#L4176 - would you be able to test if compilation works if you remove them?

Yes, it does compile.

awused commented 3 months ago

At git head it also doesn't build on Fedora 40 anymore, GCC14. I'm left in an awkward position because the vendored feature has been broken for a while, but image crate 0.25.x support requires is tied to a really new version of libjxl. I've been looking at jxl-oxide but I'd have to port all the glue code from one of the image PRs.

veluca93 commented 3 months ago

At git head it also doesn't build on Fedora 40 anymore, GCC14. I'm left in an awkward position because the vendored feature has been broken for a while, but image crate 0.25.x support requires is tied to a really new version of libjxl. I've been looking at jxl-oxide but I'd have to port all the glue code from one of the image PRs.

What's the issue on Fedora 40?

awused commented 3 months ago

It seems like it gets past the lib/lib64 problems now, or this is a new problem. Either way I've never gotten the vendored feature to work.

error: failed to run custom build command for `jpegxl-sys v0.10.3+libjxl-0.10.2 (https://github.com/inflation/jpegxl-rs#70f85b4d)`

Caused by:
  process didn't exit successfully: `/cache/rust/debug/build/jpegxl-sys-679a4090049968be/build-script-build` (exit status: 101)
  --- stdout
  CMAKE_TOOLCHAIN_FILE_x86_64-unknown-linux-gnu = None
  CMAKE_TOOLCHAIN_FILE_x86_64_unknown_linux_gnu = None
  HOST_CMAKE_TOOLCHAIN_FILE = None
  CMAKE_TOOLCHAIN_FILE = None
  CMAKE_GENERATOR_x86_64-unknown-linux-gnu = None
  CMAKE_GENERATOR_x86_64_unknown_linux_gnu = None
  HOST_CMAKE_GENERATOR = None
  CMAKE_GENERATOR = None
  CMAKE_PREFIX_PATH_x86_64-unknown-linux-gnu = None
  CMAKE_PREFIX_PATH_x86_64_unknown_linux_gnu = None
  HOST_CMAKE_PREFIX_PATH = None
  CMAKE_PREFIX_PATH = None
  CMAKE_x86_64-unknown-linux-gnu = None
  CMAKE_x86_64_unknown_linux_gnu = None
  HOST_CMAKE = None
  CMAKE = None
  running: cd "/cache/rust/debug/build/jpegxl-sys-2846afb745f246d7/out/build" && CMAKE_PREFIX_PATH="" "cmake" "/home/awused/.cargo/git/checkouts/jpegxl-rs-92c49c64cdcab3b5/70f85b4/jpegxl-src/libjxl" "-DBUILD_TESTING=OFF" "-DJPEGXL_STATIC=ON" "-DJPEGXL_ENABLE_TOOLS=OFF" "-DJPEGXL_ENABLE_DOXYGEN=OFF" "-DJPEGXL_ENABLE_MANPAGES=OFF" "-DJPEGXL_ENABLE_BENCHMARK=OFF" "-DJPEGXL_ENABLE_EXAMPLES=OFF" "-DJPEGXL_ENABLE_JNI=OFF" "-DJPEGXL_ENABLE_SJPEG=OFF" "-DJPEGXL_ENABLE_OPENEXR=OFF" "-DJPEGXL_ENABLE_JPEGLI=OFF" "-DCMAKE_INSTALL_PREFIX=/cache/rust/debug/build/jpegxl-sys-2846afb745f246d7/out" "-DCMAKE_C_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64 -march=native -pipe" "-DCMAKE_C_COMPILER=/usr/bin/cc" "-DCMAKE_CXX_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64 -march=native -pipe" "-DCMAKE_CXX_COMPILER=/usr/bin/c++" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64 -march=native -pipe" "-DCMAKE_ASM_COMPILER=/usr/bin/cc" "-DCMAKE_BUILD_TYPE=RelWithDebInfo"
  -- CMAKE_SYSTEM_PROCESSOR is x86_64
  -- tcmalloc version  -- tcmalloc 2.8.0 disabled due to https://github.com/gperftools/gperftools/issues/1204
  -- Compiled IDs C:GNU, C++:GNU
  -- Disabled AVX512 (set JPEGXL_ENABLE_AVX512 to enable it)
  -- Disabled AVX512_SPR (set JPEGXL_ENABLE_AVX512_SPR to enable it)
  -- Disabled AVX512_ZEN4 (set JPEGXL_ENABLE_AVX512_ZEN4 to enable it)
  -- Configuring incomplete, errors occurred!

  --- stderr
  CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
    Could NOT find Threads (missing: Threads_FOUND)
  Call Stack (most recent call first):
    /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
    /usr/share/cmake/Modules/FindThreads.cmake:226 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
    CMakeLists.txt:223 (find_package)

  thread 'main' panicked at /home/awused/.cargo/registry/src/index.crates.io-6f17d22bba15001f/cmake-0.1.50/src/lib.rs:1098:5:

  command did not execute successfully, got: exit status: 1

  build script failed, must exit now
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
cc --version
cc (GCC) 14.0.1 20240411 (Red Hat 14.0.1-0)
Copyright (C) 2024 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
inflation commented 3 months ago

It seems your compiler doesn't have threads support, which is surprising. libjxl uses C++ standard library for threads and etc.

awused commented 3 months ago

My compiler is fine, and I can build libjxl from source myself which is what I'm using to work around the broken vendored feature. Whatever jpegxl-rs is doing is producing a broken build environment.

inflation commented 3 months ago

My compiler is fine, and I can build libjxl from source myself which is what I'm using to work around the broken vendored feature. Whatever jpegxl-rs is doing is producing a broken build environment.

Could you try enabling JPEGXL_STATIC in CMake and see what happens?

awused commented 3 months ago

That seems to break as well, seems like it might be a GCC14/clang18 change https://gcc.gnu.org/gcc-14/porting_to.html#header-dep-changes, anyway I guess it's a libjxl bug rather than a jpegxl-rs bug.

inflation commented 3 months ago

That seems to break as well, seems like it might be a GCC14/clang18 change https://gcc.gnu.org/gcc-14/porting_to.html#header-dep-changes, anyway I guess it's a libjxl bug rather than a jpegxl-rs bug.

Try BUILD_SHARED_LIBS=OFF then, it may make some difference.

awused commented 3 months ago

cmake -DJPEGXL_STATIC=on -DBUILD_SHARED_LIBS=off runs into the same error.

inflation commented 3 months ago

cmake -DJPEGXL_STATIC=on -DBUILD_SHARED_LIBS=off runs into the same error.

Without JPEGXL_STATIC=ON

awused commented 3 months ago

Yeah that seems to build.

inflation commented 3 months ago

@veluca93 Try fix/macos branch or #57, it may solve the build problem on macOS.

@awused I've switched to that option on master.