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.

Sign up for GitHub

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

Jump to bottom

CMake: MINGW implies WIN32 #70

Merged
merged 1 commit into from
Oct 31, 2020
Merged

CMake: MINGW implies WIN32 #70

merged 1 commit into from
Oct 31, 2020

Conversation

xantares
Copy link
Contributor

hdf5 requires cmake 3, so we can simplify these

@xantares xantares mentioned this pull request Oct 30, 2020
Merged
byrnHDF
byrnHDF reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@byrnHDF byrnHDF left a comment

Choose a reason for hiding this comment

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

I'm not sure this assumption is valid, I will need to investigate as there are two use cases for mingw, 1. On windows compiling as linux and 2. On linux compiling for windows (on windows for windows would be included).

@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 30, 2020

I'm not sure this assumption is valid, I will need to investigate as there are two use cases for mingw, 1. On windows compiling as linux and 2. On linux compiling for windows (on windows for windows would be included).

Of course I am not sure about (1) as I have not tried that environment, and might be confusing it with cygwin.

@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 30, 2020

I was confusing cygwin and mingw, however:
Platform Variables
CMake sets certain variables to true depending on the current platform and toolchain in use. These always describe the target platform. In older versions of CMake, these were the only way of detecting the current platform.

UNIX : is TRUE on all UNIX-like OS's, including Apple OS X and
CygWin

WIN32 : is TRUE on Windows. Prior to 2.8.4 this included CygWin

APPLE : is TRUE on Apple systems. Note this does not imply the
system is Mac OS X, only that APPLE is #defined in C/C++
header files.

MINGW : is TRUE when using the MinGW compiler in Windows

MSYS : is TRUE when using the MSYS developer environment in Windows

CYGWIN : is TRUE on Windows when using the CygWin version of cmake

NOTE that there is not a mention of MINGW on linux, however, if you are using mingw on linux then WIN32 will not be set and that is the reason for the :if (WIN32 OR MINGW) uses.

@xantares
Copy link
Contributor Author

no, MINGW is also set when crosscompiling from linux from my experience
in the doc it is said to be set as soon as the toolchain is mingw:
https://cmake.org/cmake/help/v3.18/variable/MINGW.html?highlight=mingw

@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 30, 2020

no, MINGW is also set when crosscompiling from linux from my experience
in the doc it is said to be set as soon as the toolchain is mingw:
https://cmake.org/cmake/help/v3.18/variable/MINGW.html?highlight=mingw

Right, but on linux it will not set "WIN32" so you need to check for "MINGW" as well.

@xantares
Copy link
Contributor Author

xantares commented Oct 30, 2020 •
edited
Loading

no, as the target platform is also windows, WIN32 is also set:
https://cmake.org/cmake/help/latest/variable/WIN32.html

@xantares
Copy link
Contributor Author

maybe you're confusing host platform and target platform ?

@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 30, 2020

maybe you're confusing host platform and target platform ?

maybe I am. Is it possible to have WIN32 not defined, yet have MINGW defined. If not then I agree with these changes. We will need to test.

@xantares
Copy link
Contributor Author

afaik the mingw toolchain can only target the windows platform so WIN32 would always be set

@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 30, 2020 •
edited
Loading

afaik the mingw toolchain can only target the windows platform so WIN32 would always be set

okay, "if(WIN32 OR MINGW") == "if(WIN32)" - but now the tricky part - we need to differentiate between what is target settings vs build time settings - when building on linux, paths will need to be unix style (runtest.cmake). In this case we will need
"if(WIN32 AND NOT MINGW)" checks.

@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 30, 2020 •
edited
Loading

afaik the mingw toolchain can only target the windows platform so WIN32 would always be set

okay, "if(WIN32 OR MINGW") == "if(WIN32)" - but now the tricky part - we need to differentiate between what is target settings vs build time settings - when building on linux, paths will need to be unix style (runtest.cmake). In this case we will need
"if(WIN32 AND NOT MINGW)" checks.

Wait that's not right either, "if(MINGW AND NOT {HOST is WINDOWS})"

@xantares
Copy link
Contributor Author

In general there's no need to tweak paths because cmake uses the unix convention internally, except maybe when calling native tools.
I did run the tests and only a few failed, so I guess we're fine here.

@byrnHDF
Copy link
Contributor

byrnHDF commented Oct 30, 2020

In general there's no need to tweak paths because cmake uses the unix convention internally, except maybe when calling native tools.
I did run the tests and only a few failed, so I guess we're fine here.

That's good.
BTW, the variable is "CMAKE_HOST_SYSTEM_NAME".

I may need to setup a MINGW on Windows test machine - I haven't had much luck previously.

byrnHDF
byrnHDF reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@byrnHDF byrnHDF left a comment

Choose a reason for hiding this comment

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

Looks like java and plugin tests will need investigation because of use of path and classpath changes.
Otherwise everything looks good.

CMakeInstallation.cmake Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
config/cmake/jrunTest.cmake Show resolved Hide resolved
config/cmake/jrunTest.cmake Show resolved Hide resolved
config/cmake/jvolTest.cmake Show resolved Hide resolved
config/cmake_ext_mod/grepTest.cmake Show resolved Hide resolved
config/cmake_ext_mod/runTest.cmake Show resolved Hide resolved
byrnHDF
byrnHDF approved these changes Oct 30, 2020
View reviewed changes
Copy link
Contributor

@byrnHDF byrnHDF left a comment

Choose a reason for hiding this comment

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

These changes look good, if at some time in the future this gets tested on a windows machine with mingw and issues are found - a new PR can be discussed.

@lrknox lrknox merged commit b4ef77e into HDFGroup:develop Oct 31, 2020
@xantares xantares deleted the mingw4 branch October 31, 2020 12:20
@byrnHDF byrnHDF mentioned this pull request Nov 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derobins derobins derobins approved these changes

@lrknox lrknox lrknox approved these changes

@byrnHDF byrnHDF byrnHDF approved these changes

@bmribler bmribler Awaiting requested review from bmribler bmribler is a code owner

@fortnern fortnern Awaiting requested review from fortnern fortnern is a code owner

@jhendersonHDF jhendersonHDF Awaiting requested review from jhendersonHDF jhendersonHDF is a code owner

@jrmainzer jrmainzer Awaiting requested review from jrmainzer

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@soumagne soumagne Awaiting requested review from soumagne

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xantares @byrnHDF @derobins @lrknox