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

fix applying generator_platform based on version instead of actual generator #7684

Merged
merged 3 commits into from Sep 17, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Sep 9, 2020

Changelog: Bugfix: CMake build helper: Use actual CMake generator version to append platform generator, instead of the Conan setting compiler.version.
Docs: Omit

Close #7683

@@ -202,7 +202,7 @@ def cmake_generator_intel_test(self):
conanfile.settings = settings

cmake = CMake(conanfile)
self.assertIn('-G "Visual Studio 15 2017" -A "x64"', cmake.command_line)
self.assertIn('-G "Visual Studio 15 2017 Win64"', cmake.command_line)
Copy link
Member Author

@memsharded memsharded Sep 9, 2020

Choose a reason for hiding this comment

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

This seemed a bug, that it was using the Intel compiler version (19), instead of the actual CMake generator version

@memsharded memsharded changed the base branch from develop to release/1.29 Sep 12, 2020
@memsharded memsharded force-pushed the fix/cmake_generator_arch branch from 5db0480 to 42bc73e Compare Sep 12, 2020
@memsharded memsharded modified the milestones: 1.30, 1.29.1 Sep 12, 2020
Copy link
Contributor

@jgsogo jgsogo left a comment

Have a look to the line in the _build method, does it require the same implementation to get the compiler_version?

# the -A argument to keep previous implementation, but any modern CMake will support
# (and recommend) passing the platform in its own argument.
# Get the version from the generator, as it could have been defined by user argument
compiler_version = re.search("Visual Studio ([0-9]*)", generator).group(1)
Copy link
Contributor

@jgsogo jgsogo Sep 15, 2020

Choose a reason for hiding this comment

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

I can see the same pattern in the _build method, do we need the same change there?

Copy link
Member Author

@memsharded memsharded Sep 17, 2020

Choose a reason for hiding this comment

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

Yes, it was extremely unlikely, because it meant using VS 9 2008, but I have changed it too, and added a check in the test.

Copy link
Contributor

@jgsogo jgsogo Sep 17, 2020

Choose a reason for hiding this comment

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

I was referring to line ~320 (inside the function _build()):

compiler_version = self._settings.get_safe("compiler.version")

I think it would be enough to substitute that line with this same regex

Copy link
Member Author

@memsharded memsharded Sep 17, 2020

Choose a reason for hiding this comment

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

Not a oneliner, because if the generator is not "Visual Studio", the group() call will raise NoneType. As it is only used for Visual generator, I moved it down to the correct place. I forgot to remove this unused line, I have just fixed it.

@memsharded memsharded requested a review from jgsogo Sep 17, 2020
jgsogo
jgsogo approved these changes Sep 17, 2020
Copy link
Contributor

@jgsogo jgsogo left a comment

LGTM

I think it can be simplified #7684 (comment), but that can wait for a refactor.

@jgsogo jgsogo assigned czoido and unassigned jgsogo Sep 17, 2020
@memsharded
Copy link
Member Author

memsharded commented Sep 17, 2020

I think it can be simplified #7684 (comment), but that can wait for a refactor.

Yes, I had refactored it, but reverted the refactor as I prefer to keep the changes minimal.

jgsogo
jgsogo approved these changes Sep 17, 2020
@czoido czoido merged commit 79da939 into conan-io:release/1.29 Sep 17, 2020
2 checks passed
@memsharded memsharded deleted the fix/cmake_generator_arch branch Sep 17, 2020
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.

[bug] cmake helper is handling Visual Studio 16 2019 incorrectly
3 participants