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

[refactor] Split generator and generator platform in CMake command line when possible #6312

Merged
merged 9 commits into from Jan 27, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Jan 8, 2020

Changelog: Fix: Fixed wrong CMake command line with -G Visual Studio 15 ARM for armv8 architectures.
Docs: omit

Contains a refactor to have cmake_flags.py to define the generator platform correctly always, and the CMake build helper to undo the logic to keep the command line output unchanged.

Comes from https://github.com/conan-io/conan/pull/5919/files#r364149623, it is mostly a refactor to simplify changes in toolsets

#tags: slow

@jgsogo jgsogo changed the title Use whenever it is possible the -G and -A arguments to CMake [refactor] Split generator and generator platform in CMake command line when possible Jan 8, 2020
@jgsogo jgsogo mentioned this pull request Jan 8, 2020
9 tasks
@jgsogo jgsogo self-assigned this Jan 8, 2020
@@ -728,7 +728,7 @@ def check(text, build_config, generator=None, set_cmake_flags=False):

settings.compiler.version = "15"
settings.arch = "armv8"
check('-G "Visual Studio 15 2017 ARM" -DCONAN_IN_LOCAL_CACHE="OFF" '
check('-G "Visual Studio 15 2017" -A "ARM64" -DCONAN_IN_LOCAL_CACHE="OFF" '
Copy link
Member Author

@jgsogo jgsogo Jan 8, 2020

Choose a reason for hiding this comment

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

🧐

Copy link
Member Author

@jgsogo jgsogo Jan 8, 2020

Choose a reason for hiding this comment

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

According to our existing implementation, armv8 should be translated into ARM64, and according to CMake this platform is not supported using -G argument (https://cmake.org/cmake/help/v3.16/generator/Visual%20Studio%2015%202017.html), so we need to use -A

@@ -1052,7 +1049,7 @@ def test_clean_sh_path(self):
cmake.configure()
self.assertIn(self.tempdir, conanfile.path)

cmake.generator = "MinGW Makefiles"
cmake = CMake(conanfile, generator="MinGW Makefiles")
Copy link
Member Author

@jgsogo jgsogo Jan 9, 2020

Choose a reason for hiding this comment

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

.generator is documented, but IMO it should be a read-only property (to force/specify a different one, use the argument in the constructor), otherwise the logic implemented in get_generator_platform won't be taken into account.

This changeset introduces this regression, what to do?

  • Change only the docs to say generator and generator_platform are read-only attributes
  • Change docs and implementation to ensure that those attributes are only read-only
  • If generator is assigned to something, call again to get_generator_platform

wdyt, @memsharded ?

Copy link
Member

@memsharded memsharded Jan 27, 2020

Choose a reason for hiding this comment

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

I wouldn't change this, it would be better if it were RO, but the truth is that it is not, and we will most likely break someone if we change this one.

So I would go for the third one, if generator is assigned, call again to get_generator_platform unless the platform was set explicitly in the constructor too.

Copy link
Member

@memsharded memsharded left a comment

Looking good, please have a look at the comments.

conans/client/build/cmake.py Show resolved Hide resolved
'generator platform, or change the CMake generator.'
% self.generator)
if 'Visual Studio' in generator and self._settings.get_safe("os") != "WindowsCE":
if self.generator_platform == "x64":
Copy link
Member

@memsharded memsharded Jan 27, 2020

Choose a reason for hiding this comment

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

Maybe a dict().get() like will be a bit more explicit of the mapping.

if self.generator_platform:
if is_generator_platform_supported(self.generator):
args.append('-A "%s"' % self.generator_platform)
if self.generator == "Visual Studio 16 2019":
Copy link
Member

@memsharded memsharded Jan 27, 2020

Choose a reason for hiding this comment

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

Maybe this whole logic can be simplified. If self.generator_platform is None, it can be assigned to generator_platform anyway. Also, structure the cases top-down: first check if Visual Studio, if true, check if VS16 or WindowsCE, etc.

@jgsogo jgsogo marked this pull request as ready for review Jan 27, 2020
@memsharded memsharded added this to the 1.22 milestone Jan 27, 2020
@memsharded memsharded merged commit 6baa11b into conan-io:develop Jan 27, 2020
2 checks passed
@jgsogo jgsogo deleted the refact/get_generator branch Jan 27, 2020
@nburles
Copy link
Contributor

@nburles nburles commented Feb 14, 2020

This commit breaks our build...

conanfile.py (tm1/1.0.0): Running build()
CMake Error: Could not create named generator Visual Studio 15 2017 Win64 Win64

We use

cmake = CMake(
    self,
    cmake_system_name=self.os,
    generator="Visual Studio 15 2017 Win64" if self.os == "Windows" else "Unix Makefiles",
    toolset="v141,host=x64" if self.os == "Windows" else None
)

If I change it to use

cmake = CMake(
    self,
    cmake_system_name=self.os,
    generator="Visual Studio 15 2017" if self.os == "Windows" else unix_generator,
    generator_platform="x64",
    toolset="v141,host=x64" if self.os == "Windows" else None
)

then it works... but that will force all developers / CI machines to have to upgrade to Conan 1.22.2.

Instead, it would be better if Conan only appends the platform if the generator does not already contain it! Fixed by #6549

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Feb 14, 2020

Hi, @nburles I've seen your PR, but I want to ask you a couple of questions before.

I wonder why you need to specify all those ifs yourself, it means that the CMake build helper we provide is not able to translate your Conan profile to the CMake definitions you need. Is the problem related to CMake defaulting the x86 MSBuild even if it is a native-x64 build? Do you think Conan should force the toolset (the host part) if it is not cross-compiling?

Probably this is more something to think about for the next minor release (and your PR should be a hotfix for a patch version), but your feedback about this issue and the reason why you are writing all those conditions to use the CMake build helper is very valuable.

Thanks in advance!

@nburles
Copy link
Contributor

@nburles nburles commented Feb 14, 2020

For generator, I simplified it to be VS / Makefiles (and just noticed I forgot to do that in the second example)... we actually do different things depending on if the developer wants to simply build or if they want to open the solution in Visual Studio / CLion / etc - so rather than trying to encode that in the Conan profile (and therefore requiring that to change dependant on use-case), we encoded it the conanfile with a couple of different scripts to help out.

For toolset / host, it's not required of course, but our build is so large that it fails to link with the default x86, so the linker then re-runs with x64... it's just faster to use x64 directly (but that doesn't apply for smaller projects, the default x86 host option is preferable on speed / memory).

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.

None yet

3 participants