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

[cmake] check present "CPU" variable on PrepareEnv, if empty use CMAKE_SYSTEM_PROCESSOR #16547

Merged
merged 1 commit into from
Sep 1, 2019

Conversation

AlwinEsch
Copy link
Member

@AlwinEsch AlwinEsch commented Aug 30, 2019

Description

This is necessary to addons outside of "./tools/depends" to e.g. Mac to build because the variable "CPU" can only be given from there at the addon construction.

This is OK for cross platform addon construction like iOS or Android, but very cumbersome for building an addon to the same system (for testing purposes).

This is added:

if(NOT CPU)
  set(CPU ${CMAKE_SYSTEM_PROCESSOR})
endif()

Without comes:

<platform>osx-</platform>

With:

<platform>osx-x86_64</platform>

Motivation and Context

How Has This Been Tested?

Addon on Mac by:

cd visualization.spectrum # Where addon source is
mkdir build
cd build
/Users/Shared/xbmc-depends/x86_64-darwin18.5.0-native/bin/cmake \
   -DADDONS_TO_BUILD=visualization.spectrum \
   -DADDON_SRC_PREFIX=../..  \
   -DCMAKE_BUILD_TYPE=Release  \
   -DCMAKE_INSTALL_PREFIX="$HOME/Library/Application Support/Kodi/addons"  \
   -DPACKAGE_ZIP=1 ../../kodi/cmake/addons
make

Note: In this way, it must be present under ./Cmake/addons/addons (the content of https://github.com/xbmc/repo-binary-addons, which will be automatically downloaded), or if added subsequently there.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@AlwinEsch AlwinEsch added Type: Fix non-breaking change which fixes an issue Component: Binary add-ons CMake v19 Matrix labels Aug 30, 2019
@AlwinEsch AlwinEsch added this to the Matrix 19.0-alpha 1 milestone Aug 30, 2019
@AlwinEsch AlwinEsch requested a review from Rechi August 30, 2019 14:51
@AlwinEsch AlwinEsch changed the title [cmake] check presence "CPU" variable on PrepareEnv, if empty use CMAKE_SYSTEM_PROCESSOR [cmake] check present "CPU" variable on PrepareEnv, if empty use CMAKE_SYSTEM_PROCESSOR Aug 30, 2019
@phunkyfish
Copy link
Contributor

Just tested this. What a simple solution that will save so much time. Thanks!

Maybe just add a comment so everyone knows why it’s there!

@AlwinEsch AlwinEsch merged commit 7ad4acd into xbmc:master Sep 1, 2019
@AlwinEsch AlwinEsch deleted the fix-cpu-value branch September 1, 2019 17:34
@phunkyfish
Copy link
Contributor

@AlwinEsch can this be backported to Leia?

@phunkyfish
Copy link
Contributor

Or I can do it if you don’t have the time?

@AlwinEsch
Copy link
Member Author

AlwinEsch commented Sep 3, 2019

Better you do 😄, my car died yesterday and need to go around (by rental car) to buy a new 😡

@phunkyfish
Copy link
Contributor

Sure, happy to. Sorry to hear about your car!

@Rechi
Copy link
Member

Rechi commented Sep 3, 2019

The coorect fix would have been to set CMAKE_SYSTEM_PROCESSOR in Toolchain(_binaddons).cmake and then use that variable instead of CPU in PrepareEnv.cmake.

@phunkyfish
Copy link
Contributor

So as CMAKE_SYSTEM_PROCESSOR is already set, we could have just substituted CPU with CMAKE_SYSTEM_PROCESSOR for OSX is what you are saying?

Or is there a reason is explicitly set it in Toolchain(_binaddons).cmake?

@Rechi
Copy link
Member

Rechi commented Sep 3, 2019

CMake auto detects CMAKE_SYSTEM_PROCESSOR value, unless you use a toolchain file. That's the reason why it has to be set in those files.

@phunkyfish
Copy link
Contributor

Ok, that makes sense. I'm not the best person to make this change due to my novice cmake skills ;)

Once the correct change is in place I'm happy to test and backport it.

@wsnipex
Copy link
Member

wsnipex commented Sep 3, 2019

the change as is is not wrong either. Both work

@Rechi
Copy link
Member

Rechi commented Sep 3, 2019

Yes it works too, but this PR allows to build add-ons for osx native and not via 'cross-compiling'.
Because of that it should follow the standard CMake way by using CMAKE_SYSTEM_PROCESSOR and setting that variable if cross-compiling instead of Kodis custom CPU variable.

@phunkyfish
Copy link
Contributor

Then that’s the solution we should move towards. @AlwinEsch when you have time for a follow up PR let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Component: Binary add-ons Type: Fix non-breaking change which fixes an issue v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants