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

Only append generator_platform if needed #6549

Merged
merged 2 commits into from Mar 2, 2020

Conversation

nburles
Copy link
Contributor

@nburles nburles commented Feb 14, 2020

Changelog: Bugfix: Conan should not append generator_platform to the Visual Studio generator if it is already specified by the user.
Docs: omit

Conan should not append generator_platform to the Visual Studio generator if it is already specified by the user - otherwise this breaks builds and means all users must upgrade to use the same version of Conan (there is no way to structure the call to CMake's constructor such that it works with Conan both before and after this change).

@claassistantio
Copy link

@claassistantio claassistantio commented Feb 14, 2020

CLA assistant check
All committers have signed the CLA.

@nburles
Copy link
Contributor Author

@nburles nburles commented Feb 14, 2020

See #6312 (comment) for further information.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Feb 14, 2020

We should consider it for a patch release (how urgent it is for you?), I'll talk to the team about it.

Nevertheless, for the next major we can probably think about a more general solution in the CMake build helper (see comment in the issue: #6312 (comment)).

Thanks for reporting and contributing.

@memsharded
Copy link
Member

@memsharded memsharded commented Feb 14, 2020

Yes, I would say that both make sense:

  • Consider it for a patch release, it is a regression, could be affecting other users.
  • Learn the underlying reasons, why the default behavior is not good enough, and if it is possible to make improvements for a more general CMake helper.

@nburles
Copy link
Contributor Author

@nburles nburles commented Feb 14, 2020

We should consider it for a patch release (how urgent it is for you?)

Not super urgent for us... one of my team upgraded to the latest version, and found that they could no longer build - so have downgraded again for now, and I'll tell the others not to upgrade yet! (But inevitably someone will, and be surprised when it doesn't work! 😀)

@jgsogo jgsogo added this to the 1.22.3 milestone Feb 17, 2020
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Feb 17, 2020

Hi, @nburles

Would you mind to change the base branch of this PR to branch release/1.22.3? Feel free to cherry-pick, push, open another PR,... whatever is easier for you to get rid of changes from develop.

Also, if you don't mind, it would be nice to add some unittests to conans/test/unittests/client/build/cmake_test.py to make sure we don't break it again in Conan v1.x (if you need any help, or you prefer me to add the tests, just let me know).

Thanks!

@nburles
Copy link
Contributor Author

@nburles nburles commented Mar 2, 2020

@jgsogo Sorry, I was away when you posted the message... and completely forgot until I just got a notification of your new commit!

Do you still need me to do anything?

I see you added a unit test - but presumably still want me to rebase the PR? (Although I don't think GitHub allows changing the target of a PR, so I would indeed need to cherry-pick the commits onto a new PR targeting release/1.22.3, right?)

jgsogo
jgsogo approved these changes Mar 2, 2020
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 2, 2020

Hi! Don't worry, we're about to release 1.23 and we need to release this first 😉

According to the changes in the PR, it is modifying only these two files, so it should be ok to merge, but we'll check it.

Thanks!

@nburles
Copy link
Contributor Author

@nburles nburles commented Mar 2, 2020

OK, thanks - just let me know if anything is needed from me, and I promise I'll pay attention and actually do it 😄

@jgsogo jgsogo changed the base branch from develop to release/1.22.3 Mar 2, 2020
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 2, 2020

Oh! My fault, sorry, I thought it was targetting 1.22.3 already... I've changed it and indeed there are a lot of unwanted changes from develop.

If you can open a new branch and cherry-pick the two changes, then create a new PR just with those two, it would be nice

Thanks!

@nburles nburles force-pushed the fix_visual_studio_generator branch from a254cd0 to 6303bc5 Compare Mar 2, 2020
@nburles
Copy link
Contributor Author

@nburles nburles commented Mar 2, 2020

Huh, I see you can in fact change which branch is targeted...

So I've fixed up this PR to just include the two desired commits. EDIT: re-pushed, to fix my name / email on the commits

nburles and others added 2 commits Mar 2, 2020
Conan should not append generator_platform to the Visual Studio
generator if it is already specified by the user - otherwise this breaks
builds and means all users must upgrade to use the same version of Conan
(there is no way to structure the call to CMake's constructor such that
it works with Conan both before and after this change).
@nburles nburles force-pushed the fix_visual_studio_generator branch from 6303bc5 to 0f03028 Compare Mar 2, 2020
jgsogo
jgsogo approved these changes Mar 2, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Thanks a lot!

@jgsogo jgsogo merged commit b703390 into conan-io:release/1.22.3 Mar 2, 2020
2 checks passed
@nburles nburles deleted the fix_visual_studio_generator branch Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants