search

conda-forge / vc-feedstock

A conda-smithy repository for vc.
BSD 3-Clause "New" or "Revised" License
4 stars 22 forks source link

Add CMAKE_GENERATOR_PLATFORM #34

Closed h-vetinari closed 3 years ago

h-vetinari commented 3 years ago

Fixes #21

conda-forge-linter commented 3 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

h-vetinari commented 3 years ago

If somebody does cmake -G %CMAKE_GENERATOR%

Why would they do that (genuine question)? Since CMAKE_GENERATOR is set by the activation script (and automatically picked up by CMake), I don't see why that would make sense. Have you seen many recipes like that in the wild?

traversaro commented 3 years ago

This introduces more problems though. If somebody does cmake -G %CMAKE_GENERATOR% in the recipe, CMAKE_GENERATOR_PLATFORM is ignored.

The problem is that since Visual Studio 2019 CMake stopped provide all the possible combinations as different generators (see https://cmake.org/cmake/help/latest/generator/Visual%20Studio%2016%202019.html#platform-selection vs https://cmake.org/cmake/help/latest/generator/Visual%20Studio%2015%202017.html#platform-selection) so you can't in any case influence the platform setting by just setting the generator.

isuruf commented 3 years ago

Sure, that's true for VS 2019, but this PR is changing VS 2017 too.

traversaro commented 3 years ago

Sure, that's true for VS 2019, but this PR is changing VS 2017 too.

Ah, I got it then.

h-vetinari commented 3 years ago

I had been under the impression from @traversaro's OP in #21 that this is (CMake-)compatible across VS 2017/2019?

h-vetinari commented 3 years ago

@isuruf: Sure, that's true for VS 2019, but this PR is changing VS 2017 too.

I also checked the x-compat in the docs here [my emphasis]:

The CMAKE_GENERATOR_PLATFORM variable may be set, perhaps via the cmake(1) -A option, to specify a target platform name (architecture). For example:

  • cmake -G "Visual Studio 15 2017" -A Win32
  • cmake -G "Visual Studio 15 2017" -A x64
  • cmake -G "Visual Studio 15 2017" -A ARM
  • cmake -G "Visual Studio 15 2017" -A ARM64

For compatibility with CMake versions prior to 3.1, one may specify a target platform name optionally at the end of the generator name. This is supported only for: Visual Studio 15 2017 Win64

Specify target platform x64.

Visual Studio 15 2017 ARM

Specify target platform ARM.

h-vetinari commented 3 years ago

However, if you prefer to make activate.bat version dependent, that's not a problem for me either.

gabm commented 3 years ago

I have been bitten by the bug as well and wanted to work on a fix. Lucky I saw this PR.

Given that the change is backward compatible with vs2017, can we get this PR merged? If not, how can I help to improve?

isuruf commented 3 years ago

Given that the change is backward compatible with vs2017

It's not due to reasons I mentioned. This change should be applied only for vs2019.

traversaro commented 3 years ago

To clarify a bit, I think that what @isuruf is suggesting for maximize backward compatibility is to have this logic (pseudocode is following, @isuruf feel free to correct me):

if(vs2017)
  if(32 bit)
    set CMAKE_GENERATOR env variable to "Visual Studio 15 2017"
  elseif(64 bit)
    set CMAKE_GENERATOR env variable to "Visual Studio 15 2017 Win64"
  endif()
elseif(vs2019)
  if(32 bit)
    set CMAKE_GENERATOR env variable to to "Visual Studio 16 2019"
    set CMAKE_PLATFORM env variable to to "Win32"
  elseif(64 bit)
    set CMAKE_GENERATOR env variable to to "Visual Studio 16 2019"
    set CMAKE_PLATFORM env variable to to "x64"
  endif()
endif()
traversaro commented 3 years ago

To clarify a bit, I think that what @isuruf is suggesting for maximize backward compatibility is to have this logic (pseudocode is following, @isuruf feel free to correct me):

if(vs2017)
  if(32 bit)
    set CMAKE_GENERATOR env variable to "Visual Studio 15 2017"
  elseif(64 bit)
    set CMAKE_GENERATOR env variable to "Visual Studio 15 2017 Win64"
  endif()
elseif(vs2019)
  if(32 bit)
    set CMAKE_GENERATOR env variable to to "Visual Studio 16 2019"
    set CMAKE_PLATFORM env variable to to "Win32"
  elseif(64 bit)
    set CMAKE_GENERATOR env variable to to "Visual Studio 16 2019"
    set CMAKE_PLATFORM env variable to to "x64"
  endif()
endif()

Great, I did not see that this was implemented in https://github.com/conda-forge/vc-feedstock/pull/35 by @gabm .

h-vetinari commented 3 years ago

Closing as superseded by #35

gabm commented 3 years ago

yes, sorry - i should have mentioned that here...