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

CMake: Enable plugins on MinGW #2914

Merged
merged 1 commit into from
May 6, 2024
Merged

CMake: Enable plugins on MinGW #2914

merged 1 commit into from
May 6, 2024

Conversation

MehdiChinoune
Copy link
Contributor

They could be built on MinGW.

@CLAassistant
Copy link

CLAassistant commented May 1, 2024

CLA assistant check
All committers have signed the CLA.

@DWesl
Copy link
Contributor

DWesl commented May 2, 2024

Are you planning to add CMake to the MinGW CI run?

You could borrow from the Linux CMake workflow, or from my most recent attempt to get CMake working on MinGW (compile fails).

@MehdiChinoune
Copy link
Contributor Author

Are you planning to add CMake to the MinGW CI run?

You could borrow from the Linux CMake workflow, or from my most recent attempt to get CMake working on MinGW (compile fails).

I was planning, but I prefered to add it in a separate PR.

@DWesl
Copy link
Contributor

DWesl commented May 3, 2024

That's a sensible concern

@MehdiChinoune
Copy link
Contributor Author

That's a sensible concern

I don't understand?

@DWesl
Copy link
Contributor

DWesl commented May 3, 2024

Trying to keep a PR focused on a single change, rather than allowing its scope to expand to include many tangentially-related things, is a sensible concern.

Copy link
Member

@WardF WardF left a comment

Choose a reason for hiding this comment

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

@DennisHeimbigner These changes largely look good to me, but I'm uncertain of the effect the change to WIN32 from MSVC will have in cygwin-based builds. You work in Cygwin much more frequently than I do; if Cygwin sets Win32 but uses Unix-style library names, this could cause a problem (I suspect). All of this is untested conjecture, and I can't easily test cygwin at the moment. Would you mind taking a look and approving these changes if they don't cause a problem?

@MehdiChinoune
Copy link
Contributor Author

I think WIN32 is not set to true on CYGWIN by CMake.

@DWesl
Copy link
Contributor

DWesl commented May 3, 2024

Cygwin CMake spent a few years telling me it no longer set _WIN32 on Cygwin, so I suspect it's a separate thing.

A small CMakeLists.txt set to output messages if WIN32 or CYGWIN is defined outputs the Cygwin message but not the Windows one.

@DennisHeimbigner
Copy link
Collaborator

Are we talking about "WIN32" or "_WIN32"? The latter is set by the compiler.
As far as I know, we never use "WIN32" anywhere in our code; we only use "_WIN32".
Also, specifying MSVC is sometimes the correct test (vs _WIN32) when the issue is
building on windows using gcc or clang vs building using the visual studio compiler.

@MehdiChinoune
Copy link
Contributor Author

We are talking about WIN32 in CMake language.

@WardF WardF merged commit f5c9183 into Unidata:main May 6, 2024
103 checks passed
@MehdiChinoune MehdiChinoune deleted the mingw-plugins branch May 6, 2024 21:23
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.

5 participants