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.

Sign up for GitHub

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

Jump to bottom

CMake consistency fixes #41

Open
wants to merge 3 commits into
base: master/s-line
Choose a base branch
from
Open

CMake consistency fixes #41

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b48ae7c
Use modern cmake >= 3.12 way of finding Python
jellespijker Mar 24, 2022
1950107
No need to exclude __pychache__
jellespijker Mar 24, 2022
e0f2f53
Merge pull request #40 from Ultimaker/CURA-8640_PyQt6_upgrade
Ghostkeeper Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 18 additions & 29 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,27 @@
project(charon NONE)
cmake_minimum_required(VERSION 3.6) #Tested only with 3.6.1 and 3.9.1.
cmake_minimum_required(VERSION 3.18)

# FIXME: Remove the code for CMake <3.12 once we have switched over completely.
# FindPython3 is a new module since CMake 3.12. It deprecates FindPythonInterp and FindPythonLibs.
if(${CMAKE_VERSION} VERSION_LESS 3.12)
# Use FindPythonInterp and FindPythonLibs for CMake <3.12
find_package(PythonInterp 3.4 REQUIRED)

set(Python3_EXECUTABLE ${PYTHON_EXECUTABLE})
set(Python3_VERSION_MAJOR ${PYTHON_VERSION_MAJOR})
set(Python3_VERSION_MINOR ${PYTHON_VERSION_MINOR})
else()
# Use FindPython3 for CMake >=3.12
find_package(Python3 ${CURA_PYTHON_VERSION} EXACT REQUIRED COMPONENTS Interpreter)
if(NOT DEFINED Python_VERSION)
set(Python_VERSION
3.10
Copy link
Member

Choose a reason for hiding this comment

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

we have 3.7 on the printer, anything above that won't work for firmware.

Copy link
Contributor

@CoenSchalkwijk CoenSchalkwijk Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, I'm assuming libCharon needs 3.10 when used with/for Cura?
If so, to be on the safe side: do not use a default value, but require Python_VERSION to be specified instead?
In order to prevent 'silent' mistakes.

CACHE STRING "Python Version" FORCE)
message(STATUS "Setting Python version to ${Python_VERSION}. Set Python_VERSION if you want to compile against an other version.")
endif()
if(APPLE)
CoenSchalkwijk marked this conversation as resolved.
Show resolved Hide resolved
set(Python_FIND_FRAMEWORK NEVER)
endif()
find_package(Python ${Python_VERSION} EXACT REQUIRED COMPONENTS Interpreter)
message(STATUS "Linking and building ${project_name} against Python ${Python_VERSION}")
if(NOT DEFINED Python_SITELIB_LOCAL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where/how's this expected to be set now?

Copy link
Contributor Author

@Ghostkeeper Ghostkeeper May 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Cura-build sets this variable upon building. The reason is that all other packages are installed to site-packages on all platforms (Windows, MacOS and Linux) but on Debian-based platforms like our Ubuntu build server the default for Python_SITELIB is dist-packages.

We added this so that from cura-build we could override the installation directory to be consistent so that we would have to make fewer exceptions.
The default remains the Python_SITELIB that becomes dist-packages on Debian-based systems, like it was before.

Copy link
Contributor

@CoenSchalkwijk CoenSchalkwijk May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since these changes are aimed at changes/requirements from the cura-build, is it stil possible to use this for a cura-build-less build of libCharon? As in: does Team Styx now need to use the cura-build to release/create a new pkg for libCharon?

Copy link
Contributor Author

@Ghostkeeper Ghostkeeper May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No, that's not our intention. That would be very unworkable for you!

If Python_SITELIB_LOCAL is not specified it should default to the old behaviour.

Would it be acceptable if we add documentation to this about that this parameter exists and leave the behaviour as-is, then?

set(Python_SITELIB_LOCAL
"${Python_SITELIB}"
CACHE PATH "Local alternative site-package location to install Cura" FORCE)
endif()

option(INSTALL_SERVICE "Install the Charon DBus-service" ON)
option(INSTALL_CLIENT "Install the Charon Client library" ON)
Copy link
Contributor

@CoenSchalkwijk CoenSchalkwijk Apr 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

With the exclusions removed, do these still matter?
INSTALL_CLIENT seems to be unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think these could be removed as well then, if they are indeed unused now.

I'm not sure why those excludes were removed because this is code for the firmware-side.


if(EXISTS /etc/debian_version)
set(CHARON_INSTALL_PATH lib${LIB_SUFFIX}/python${Python3_VERSION_MAJOR}/dist-packages)
else()
set(CHARON_INSTALL_PATH lib${LIB_SUFFIX}/python${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR}/site-packages)
endif()

set(_excludes PATTERN __pycache__ EXCLUDE)
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer necessary?

if(NOT INSTALL_SERVICE)
set(_excludes ${_excludes} PATTERN "Service" EXCLUDE)
endif()
if(NOT INSTALL_CLIENT)
set(_excludes ${_excludes} PATTERN "Client" EXCLUDE)
endif()

install(DIRECTORY Charon DESTINATION ${CHARON_INSTALL_PATH} ${_excludes})
install(DIRECTORY Charon DESTINATION "${Python_SITELIB_LOCAL}")

if(INSTALL_SERVICE)
install(FILES service/charon.service DESTINATION lib/systemd/system)
Expand All @@ -59,7 +48,7 @@ endif()

add_test(
NAME pytest-main
COMMAND ${Python3_EXECUTABLE} -m pytest --junitxml=${CMAKE_BINARY_DIR}/junit-pytest-main.xml ${CMAKE_SOURCE_DIR}/tests
COMMAND ${Python_EXECUTABLE} -m pytest --junitxml=${CMAKE_BINARY_DIR}/junit-pytest-main.xml ${CMAKE_SOURCE_DIR}/tests
CoenSchalkwijk marked this conversation as resolved.
Show resolved Hide resolved
)
set_tests_properties(pytest-main PROPERTIES ENVIRONMENT LANG=C)
set_tests_properties(pytest-main PROPERTIES ENVIRONMENT "PYTHONPATH=${_PYTHONPATH}")