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

Windows: tell cmake it's not crosscompiling in MSYS2 #4339

Closed
wants to merge 1 commit into from

Conversation

iDunk5400
Copy link
Contributor

This fixes a cmake error in MSYS2 introduced by #4294

This fixes a cmake error in MSYS2 introduced by monero-project#4294
@moneromooo-monero
Copy link
Collaborator

While I know sfa about cmake/windows, this seems unconditional. Will this actually break cross compiling ?

@iDunk5400
Copy link
Contributor Author

That's why I used the CMAKE_HOST_WIN32 check, so if host is not Windows it should still work.

@moneromooo-monero
Copy link
Collaborator

OK, but that doesn't mean the host is different from the target, does it ?

@iDunk5400
Copy link
Contributor Author

How would you interpret this ?

CMake Warning at translations/CMakeLists.txt:35 (message):
  CrossCompiling


CMake Error at translations/CMakeLists.txt:37 (INCLUDE):
  INCLUDE could not find load file:

    C:/msys64/home/idunk/monero/build/release/translations/ImportExecutables.cmake

That's without the patch.

https://cmake.org/cmake/help/v3.0/variable/CMAKE_CROSSCOMPILING.html

@moneromooo-monero
Copy link
Collaborator

I interpret this as "I'm going to cross compile". My point, however, is that I read the change as "if the host is win32, assume we're not cross compiling", without checking the target, which seems wrong unless there's a subtlety I'm missing.

@iDunk5400
Copy link
Contributor Author

iDunk5400 commented Sep 4, 2018

Those cmake toolchain files are only used in release-static-win32 and release-static-win64 targets. Maybe I'm missing something ?

EDIT: And their debug equivalents.

@moneromooo-monero
Copy link
Collaborator

Ah, that does explain then, the target is already fixed :)

Thanks

@iDunk5400
Copy link
Contributor Author

Hmm, another way of fixing this would be to remove this line. Then CMAKE_SYSTEM and CMAKE_HOST_SYSTEM would match, and CMAKE_CROSSCOMPILING would not be set.

Relevant cmake vars with L29:

-- CMAKE_HOST_SYSTEM=Windows-10.0.17134
-- CMAKE_HOST_SYSTEM_NAME=Windows
-- CMAKE_HOST_SYSTEM_PROCESSOR=AMD64
-- CMAKE_HOST_SYSTEM_VERSION=10.0.17134

-- CMAKE_SYSTEM=Windows
-- CMAKE_SYSTEM_NAME=Windows
-- CMAKE_SYSTEM_PROCESSOR=
-- CMAKE_SYSTEM_VERSION=
-- CMAKE_CROSSCOMPILING=TRUE

Relevant cmake vars without L29:

-- CMAKE_HOST_SYSTEM=Windows-10.0.17134
-- CMAKE_HOST_SYSTEM_NAME=Windows
-- CMAKE_HOST_SYSTEM_PROCESSOR=AMD64
-- CMAKE_HOST_SYSTEM_VERSION=10.0.17134

-- CMAKE_SYSTEM=Windows-10.0.17134
-- CMAKE_SYSTEM_NAME=Windows
-- CMAKE_SYSTEM_PROCESSOR=AMD64
-- CMAKE_SYSTEM_VERSION=10.0.17134
-- CMAKE_CROSSCOMPILING=FALSE

@MoroccanMalinois
Copy link
Contributor

grep -R CMAKE_CROSSCOMPILING Source in cmake source code shows that it's only used to automatically skip execution of binaries, with this exception, in a function named "SetPermissions" :

#ifdef WIN32
  if (Makefile->IsOn("CMAKE_CROSSCOMPILING")) {
    // Store the mode in an NTFS alternate stream.
    std::string mode_t_adt_filename =
      std::string(toFile) + ":cmake_mode_t";

    // Writing to an NTFS alternate stream changes the modification
    // time, so we need to save and restore its original value.
    [...]

I understood it as a way to preserve file permissions when they will be moved to windows, but this could be completely wrong!

So, imho, CMAKE_SYSTEM_NAME Windows should only be set when using mingw on linux, target that doesn't exists in current Makefile (wondering if it just needs a different MSYS2_FOLDER ...)

tl;dr i vote for @iDunk5400 second solution

@iDunk5400
Copy link
Contributor Author

I agree that not setting CMAKE_SYSTEM_NAME would be the right solution. Closing this in favor of #4342.

@iDunk5400 iDunk5400 closed this Sep 6, 2018
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