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

Only pass .asm files to masm #755

Merged
merged 1 commit into from
Nov 23, 2022
Merged

Only pass .asm files to masm #755

merged 1 commit into from
Nov 23, 2022

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Nov 23, 2022

This changes the handling of assembly on MSVC targets so that only .asm files are passed to masm. This is more in line with how things were handled in cc <1.0.77, and would have avoided #754.

@thomcc thomcc requested a review from ChrisDenton November 23, 2022 06:09
@thomcc

This comment was marked as resolved.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

Effectively restoring the previous behaviour to prevent unintentional breakages looks good to me. I only have a few minor nits.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@thomcc thomcc force-pushed the asm-ext branch 3 times, most recently from 328c2dc to 78974fc Compare November 23, 2022 09:40
@thomcc

This comment was marked as outdated.

@thomcc thomcc requested a review from ChrisDenton November 23, 2022 16:15
src/lib.rs Outdated Show resolved Hide resolved

Verified

This commit was signed with the committer’s verified signature.
thomcc Thom Chiovoloni
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@MarijnS95
Copy link

and would have avoided #754.

Fwiw this is more-so caused by the zstd-sys build script looking at host OS rather than target OS, while cross-compiling. However, it seems that its choice to include this file comes down to compiler support, so if the cross-compiler was having clang-cl available that might explain why it was previously building fine.

@thomcc
Copy link
Member Author

thomcc commented Nov 23, 2022

Fwiw this is more-so caused by the zstd-sys build script looking at host OS rather than target OS, while cross-compiling.

Yeah, the point is more: you should actually be allowed to compile that file with clang-cl, even if the logic you used to decide whether or not to include it was buggy.

@thomcc thomcc deleted the asm-ext branch November 23, 2022 23:33
@MarijnS95
Copy link

@thomcc is this still on track to be released? We're currently failing to compile tract-linalg because it detects ml64.exe isn't available and passes .S files to cc, which ends up invoking ml64.exe anyway.

https://github.com/sonos/tract/blob/main/linalg/build.rs

@thomcc
Copy link
Member Author

thomcc commented Dec 14, 2022

Ah, thanks for the reminder, I'll do a release as soon as possible.

@thomcc
Copy link
Member Author

thomcc commented Dec 15, 2022

@MarijnS95 Sorry for the delay, it's in https://github.com/rust-lang/cc-rs/releases/tag/1.0.78.

@thomcc
Copy link
Member Author

thomcc commented Sep 15, 2023

A wrinkle here is that meson projects are expected to use .asm file as the file extension, regardless of assembler. I wonder if this will cause issues for us...

@ChrisDenton
Copy link
Member

Oh hm. Maybe! We probably should have ways to override any and all heuristics in any case.

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

4 participants