Skip to content

Conversation

@CrazyboyQCD
Copy link

Related Issues

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Does this PR change existing behavior?

  • Yes (please describe the changes below)
    • Remove -lm flag for building msvc target
  • No

Does this PR introduce new dependencies?

  • No
  • Yes (please check binary size changes)

Checklist:

  • [] Tests added/updated for bug fixes or new features
  • [] Compatible with Windows/Linux/macOS

@semanticdiff-com
Copy link

semanticdiff-com bot commented Jul 24, 2025

Review changes with  SemanticDiff

Changed Files
File Status
  crates/moonutil/src/compiler_flags.rs  77% smaller

@CrazyboyQCD CrazyboyQCD force-pushed the fix-msvc-native-build branch 2 times, most recently from c7be85b to d00b4a7 Compare July 24, 2025 09:23
@lynzrand
Copy link
Collaborator

Thanks for your PR!

We're sorry about the current condition of our codebase that made things unclear, but the implementation in this PR is a little off. The distinction of MSVC and GCC needs to be done at runtime instead of compile-time, so cfg! macros don't work here. You can check the following code for reference on how to detect MSVC while we're sorting things out.

// note: this is a workaround for windows cl, x86_64-pc-windows-gnu also need to consider
.args_with_cond(
cfg!(target_os = "windows")
&& moonc_opt.link_opt.target_backend == TargetBackend::LLVM
&& CC::default().is_msvc(),
["-llvm-target", "x86_64-pc-windows-msvc"],
)

@CrazyboyQCD CrazyboyQCD force-pushed the fix-msvc-native-build branch from d00b4a7 to 67315e4 Compare July 25, 2025 01:38
@CrazyboyQCD CrazyboyQCD changed the title fix: remove -lm flag for msvc target fix: remove -lm flag for windows native target Jul 25, 2025
@CrazyboyQCD
Copy link
Author

CrazyboyQCD commented Jul 25, 2025

Thanks for your PR!

We're sorry about the current condition of our codebase that made things unclear, but the implementation in this PR is a little off. The distinction of MSVC and GCC needs to be done at runtime instead of compile-time, so cfg! macros don't work here. You can check the following code for reference on how to detect MSVC while we're sorting things out.

// note: this is a workaround for windows cl, x86_64-pc-windows-gnu also need to consider
.args_with_cond(
cfg!(target_os = "windows")
&& moonc_opt.link_opt.target_backend == TargetBackend::LLVM
&& CC::default().is_msvc(),
["-llvm-target", "x86_64-pc-windows-msvc"],
)

I test that clang also fails to link m.lib, and as comment in #861 (comment) said, windows doesn't need this flag not just for msvc target.

@lynzrand
Copy link
Collaborator

Okay, I have just verified that both MSVC and GNU targets on Windows have math functions defined in MSVCRT. We will discuss this change internally before proceeding.

@lynzrand
Copy link
Collaborator

At the meantime, you might want to sign the CLA to make your changes mergable :)

Check this CI for more details: https://github.com/moonbitlang/moon/actions/runs/16511697077/job/46694762934?pr=944

@lynzrand lynzrand self-assigned this Jul 25, 2025
@lynzrand lynzrand added the bug Something isn't working label Jul 25, 2025
@CrazyboyQCD CrazyboyQCD force-pushed the fix-msvc-native-build branch from 67315e4 to 0b3367d Compare July 25, 2025 03:56
@CrazyboyQCD

This comment was marked as resolved.

@CrazyboyQCD CrazyboyQCD force-pushed the fix-msvc-native-build branch from 0b3367d to a3840f6 Compare July 25, 2025 08:50
@lynzrand lynzrand force-pushed the fix-msvc-native-build branch from 041cd10 to 0997e0b Compare August 21, 2025 02:28
@lynzrand
Copy link
Collaborator

Do note that if we're doing cross-compilation in the future, we can't use a compile-time cfg! as condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants