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

zstd-sys: Don't use target* cfg in build.rs to fix Windows cross-compile #176

Conversation

MarijnS95
Copy link
Contributor

This issue got exposed with CC upgrading to 1.0.77 and now detecting the .S file extension to pass on to ml64.exe (MASM) on Windows; we don't have that on our cross-compiling (from Linux to Windows) CI setup, hence uncovered that this build script was using the target_os flag from the host in the build script to make a decision on what to compile for the target.


Curiously this implies the .S file must have compiled fine for us on cc 1.0.76 and prior, or somehow got skipped (expecting errors when ZSTD_DISABLE_ASM isn't defined...).

…compile

This issue got exposed with [CC upgrading to 1.0.77] and now detecting
the `.S` file extension to pass on to `ml64.exe` (MASM) on Windows;
we don't have that on our cross-compiling (from Linux to Windows) CI
setup, hence uncovered that this build script was using the _`target_os`
flag from the host_ in the build script to make a decision on _what to
compile for the target_.

[CC upgrading to 1.0.77]: rust-lang/cc-rs@1.0.76...1.0.77
@gyscos
Copy link
Owner

gyscos commented Nov 22, 2022

Indeed, thanks for identifying and fixing this issue!

@gyscos
Copy link
Owner

gyscos commented Nov 22, 2022

Indeed as found in rust-lang/cc-rs#754 the compiler was previously ignoring .s files, only looking at .asm.

@Hainish
Copy link

Hainish commented Nov 22, 2022

Is it possible to make a bugfix release with this fix? This library lies upstream of a cross-compiled project I'd like to make a release for.

@gyscos
Copy link
Owner

gyscos commented Nov 23, 2022

Just published zstd-sys 2.0.2.

@MarijnS95 MarijnS95 deleted the buildrs-env-instead-of-target-cfg-for-cross-compile branch November 23, 2022 13:53
@MarijnS95
Copy link
Contributor Author

As per rust-lang/cc-rs#754 (comment) I am in hindsight not entirely sure if this is the right solution: since it was compiling just fine before in a cross-compilation setup, were we perhaps having an advantage because clang-cl is available and could compile the ASM even for Windows targets?

If that's true, perhaps we should instead check/force the compiler to be used when cross-compiling, instead of disabling it outright because of expecting ml64.exe to be used.

If anything cc just backed out of the change and now uses clang-cl again for .S files (although the community doesn't seem to have agreed yet on that distinction) like it did before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants