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

Remove mingw toolset #2396

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

Jarod42
Copy link
Contributor

@Jarod42 Jarod42 commented Dec 26, 2024

What does this PR do?

It removes toolset mingw
which is mostly an alias of "gcc on windows" (for filtering).

How does this PR change Premake's behavior?

filter { "toolset:mingw" } is broken, and alternative should be used, as filter { "system:windows", "toolset:not msc" }

Anything else we should know?

  • Detecting correct mingw-w64/llvm-mingw/msys2 is currently not done anyway
  • Some mingw uses clang and not gcc

So some works should be done if we want to handle mingw correctly.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

@@ -312,8 +312,8 @@
links { "ole32", "ws2_32", "advapi32", "version" }
files { "src/**.rc" }

filter "toolset:mingw"
links { "crypt32", "bcrypt" }
filter { "system:windows", "toolset:not msc" }
Copy link
Member

Choose a reason for hiding this comment

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

What about LLVM with Visual Studio? How should this be handled?

Copy link
Contributor Author

@Jarod42 Jarod42 Dec 27, 2024

Choose a reason for hiding this comment

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

So big build matrix! It seems I would have to add extra CI config.

I honestly don't know

  • if clang under visual should have the link
  • if clang under mingw should have the link.
    It seems to be usage from curl.

I can use filter { "system:windows", "toolset:gcc" } to not change those configs

Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like a bigger change like we discussed, does this solve an immediate problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussion was adding extra API which would be a big change IMO.
Here I (propose to) remove the strange toolset mingw (which is no even web documented)

@Jarod42 Jarod42 marked this pull request as draft December 29, 2024 09:37
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