-
Notifications
You must be signed in to change notification settings - Fork 12
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
Vendored build seems to fail on Windows and Mac OS #51
Comments
The Windows one is that you are linking with a different MSVC runtime library. By default, it will use |
The macOS one is unfortunately a limitation of Rust. You need to manually set |
Excuse my ignorance here :-) isn't defining that variable something that the build script should do? |
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. |
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
<#51 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOPAIZRM3MGAB7E6WMRDGLZAX5VNAVCNFSM6AAAAABHGZQSGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUG4YTINZVGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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: jpegxl-rs/jpegxl-src/src/lib.rs Line 42 in 78bd163
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? |
I don't think so. |
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? |
Already using in |
Ah, sorry, I didn't see that - I was confused by the fact that cmake-rs seems to have logic to handle I do not quite understand why it is being explicitly set here for Windows builds then - is the behaviour of |
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. |
After some investigation, it seems that the underlying |
#53 should fix the Windows build issue for now. I still haven't found a better solution for macOS. The problem is that |
Great news!
What does libjxl need from |
|
I see, going from the error log I assume the issues to be cause by the calls to |
Yes, it does compile. |
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? |
It seems like it gets past the lib/lib64 problems now, or this is a new problem. Either way I've never gotten the
|
It seems your compiler doesn't have threads support, which is surprising. |
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 |
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 |
|
Without |
Yeah that seems to build. |
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 :)
The text was updated successfully, but these errors were encountered: