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

fix(flag_check): never link to avoid false positives #839

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

RubixDev
Copy link
Contributor

@RubixDev RubixDev commented Aug 1, 2023

Using flag_if_supported when targeting wasm32-unknown-unknown often (or maybe always) results in false negatives, causing supported flags to not be applied. This happens, because clang is trying to link the binary with wasm-ld which must be installed, and even with wasm-ld installed clang still gives me this error:

wasm-ld: error: unknown file type: /lib/crt1.o
wasm-ld: error: cannot open /usr/lib/clang/15.0.7/lib/libclang_rt.builtins-wasm32.a: No such file or directory
clang-15: error: linker command failed with exit code 1 (use -v to see invocation)

This is easily avoided by telling clang to skip linking by using -c.

src/lib.rs Outdated Show resolved Hide resolved
@RubixDev RubixDev changed the title fix(flag_check): don't link when targeting wasm32-unknown-unknown fix(flag_check): never link to avoid false positives Aug 1, 2023
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this unconditionally does worry me. I do see @dot-asm's arguments for why it's correct, but it is still somewhat worrisome.

I think I'll take this PR, but I wouldn't be surprised if we end up needing to make this conditional again due to issues in the release.

@thomcc thomcc merged commit 9b569ae into rust-lang:main Nov 3, 2023
16 checks passed
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.

3 participants