Skip to content
This repository has been archived by the owner on Sep 5, 2022. It is now read-only.

[WIP] Cura 8640 PyQt6 upgrade #126

Merged
merged 216 commits into from
Apr 14, 2022
Merged

[WIP] Cura 8640 PyQt6 upgrade #126

merged 216 commits into from
Apr 14, 2022

Conversation

jellespijker
Copy link
Member

@jellespijker jellespijker commented Apr 12, 2022

In order to get this to work on our build-system and working for all three OSes we did a shit tons of boy scouting in our cmake. We removed old methods with variables and try to be consisted in a target-based approach. The idea is that we don't patch stuff down the line, but that the install should place everything with desired rpaths for each project.

Most of the changes had to do with how dependency targets were named and to make sure that external projects weren't downloaded automatically.

Part of

Fixes

Todo

  • Update Readme. But maybe best done in a separate ticket to prioritize with our other work for the 5.0 release. See Please update README #121 and CURA-9047
  • Make sure the additional VC redist files are installed as described in Cura 8640 PyQt6 upgrade cura-binary-data#17
  • Check and fix if confirmed CURA-9105 Uninstall Cura.exe doesn't uninstall the program
  • Check and fix if confirmed CURA-9103 Order of files in installation folder scrambled
  • Clean up and remove setting the branches and versioning in the conanfile.py
  • Remove Windows Docker files since we now build directly on a VM with Visual Studio 2019 and/or Visual Studio 2020 compiler.
  • Remove old environment scripts
  • Remove Jenkinsfile
  • Create a cmake target which allows the installation of a virtual environment (excluding Cura, Uranium and CuraEngine)
  • Create installer for MSI. Might be best to do this in a seperate ticket after the 5.0 release

Vagos Trantos and others added 30 commits January 21, 2022 16:15
We will now install Qt via pip, so this patch is not needed anymore.
We will now install PyQt6 via pip, so this project is not needed anymore.
We will now install PyQt6-Qt6 via pip, so this project is not needed anymore.
part of CURA-7924
part of CURA-7924
The linux builds are always taking a long time. This is mostly
due to `make` using a single core by default. By adding the
argument `-j $(nproc)` to various projects which are build from
source we can speed up the build process significantly.
Debian comes with a version of cmake 3.13.x
building CMake from source ensures that we can
use the latest version.
Otherwise the file(s) should be manually copied, but that'll require some trial-and-error.

part of CURA-7924
part of CURA-7924
That typo caused a lot of head-scratching. Sorry!

par of CURA-7924
@jellespijker jellespijker changed the title Cura 8640 PyQt6 upgrade [WIP] Cura 8640 PyQt6 upgrade Apr 12, 2022
Contributes to CURA-8640
Added a cmake target create-dev-env this will install
a virtual environment in a directory of choice.
with everything except Cura, Uranium, CuraEngine and
cura-binary-data.

This target can be used to setup a quick dev env for
our most changed projects.

Contributes to CURA-8640
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
Comment on lines 56 to 63
GetFromEnvironmentOrCache(
NAME
CURA_VERSION_BUILD
DESCRIPTION
"Cura Version build")
set(CURA_VERSION_EXTRA )
if(NOT ${CURA_VERSION_PRE_RELEASE_TAG} STREQUAL "")
set(CURA_VERSION_EXTRA "-${CURA_VERSION_PRE_RELEASE_TAG}+${CURA_VERSION_BUILD}")
Copy link
Contributor

Choose a reason for hiding this comment

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

This description doesn't describe anything, really.

Knowing how it's used, I think it's better to go back to using timestamps for this. It would end up something like this, I reckon (untested):

Suggested change
GetFromEnvironmentOrCache(
NAME
CURA_VERSION_BUILD
DESCRIPTION
"Cura Version build")
set(CURA_VERSION_EXTRA )
if(NOT ${CURA_VERSION_PRE_RELEASE_TAG} STREQUAL "")
set(CURA_VERSION_EXTRA "-${CURA_VERSION_PRE_RELEASE_TAG}+${CURA_VERSION_BUILD}")
option(ADD_TIMESTAMP "Add a timestamp to the version number." ON)
set(build_time "")
if(ADD_TIMESTAMP)
string(TIMESTAMP build_time "+%Y-%m-%d %H:%M:%S" UTC)
endif()
set(CURA_VERSION_EXTRA "")
if(NOT ${CURA_VERSION_PRE_RELEASE_TAG} STREQUAL "")
set(CURA_VERSION_EXTRA "-${CURA_VERSION_PRE_RELEASE_TAG}${build_time}"

Copy link
Member Author

Choose a reason for hiding this comment

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

The build metadata tag in SemVer is most commonly a increasing version number. But after reading https://semver.org/

Build metadata MAY be denoted by appending a plus sign and a series of dot separated identifiers immediately following the patch or pre-release version. Identifiers MUST comprise only ASCII alphanumerics and hyphens [0-9A-Za-z-]. Identifiers MUST NOT be empty. Build metadata MUST be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence. Examples: 1.0.0-alpha+001, 1.0.0+20130313144700, 1.0.0-beta+exp.sha.5114f85, 1.0.0+21AF26D3—-117B344092BD.

And since we now have to manually increase this number in a rundeck job I think a timestamp might be a good intermittent solution before our switch to Conan and Pipelines.

Special care should be taken to make sure that timestamp is used for all three OSes. that requires us to set it in Rundeck and not here.

@evtrados Can you make sure that all three build jobs on Rundeck use the same timestamp ad the CURA_VERSION_BUILD

Comment on lines +2 to +3

**WIP**
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this can be removed, although it will always be WIP.

Suggested change
**WIP**

README.md Outdated Show resolved Hide resolved
README.md Outdated
We can then build the environment with:
```bash
cd <root_cura_build_environment>
docker run cura-env-builder:cura-8640 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this tag name shouldn't be permanent. I don't know if you intended to change this once it was merged?

Copy link
Member Author

Choose a reason for hiding this comment

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

README.md needs a big revision this is no longer used as such but we already have CURA-9047 for this

README.md Outdated
Note that a fairly recent C++ compiler is required, at the very least GCC
4.9. If you are building on CentOS, you can get this from the devtoolset
packages.
This repository also contains dockerfiles which will build a cura-builder image for either Windows or Linux. These images
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows has been removed.

Suggested change
This repository also contains dockerfiles which will build a cura-builder image for either Windows or Linux. These images
This repository also contains dockerfiles which will build a cura-builder image for Linux. These images

README.md Outdated
4.9. If you are building on CentOS, you can get this from the devtoolset
packages.
This repository also contains dockerfiles which will build a cura-builder image for either Windows or Linux. These images
contain all build essentials to both build cura-build-environment and cura-build
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
contain all build essentials to both build cura-build-environment and cura-build
contain all build essentials to build Cura.

projects/cura.cmake Outdated Show resolved Hide resolved
projects/cura.cmake Outdated Show resolved Hide resolved
projects/curaengine.cmake Outdated Show resolved Hide resolved
@jellespijker
Copy link
Member Author

jellespijker commented Apr 13, 2022

Thanks @Ghostkeeper

TLDR; I'm reluctant to implement your bigger changes. Certainly given the time constraints we are already facing. I will change the smaller comments and I think @evtrados should implement the build version number on the Rundeck side

Yes you're correct with regards in the Dockerfile. These are based on the Conan Docker tools and could be simplified for our purpose. Something which I would certainly do if this repo still has a long life in front of it. But because we're switching to a new build setup with Conan as dependency manager and pipelines to build the dependencies and store them on an artifact server. All artifacts will be build on VM's instead of using a Docker Image.

The same goes with the readme We should probably update this, certainly after the beta release. But it will be very minimal.
Basically stating that CMake needs to be called with: CMAKE_PREFIX_PATH and CMAKE_INSTALL_PREFIX since that will be basically what we do on the build servers as well. Okay I will also state the minimum requirements: CMake, Conan, Python, GCC, Apple-Clang or MSVC but no elaborate docs.

I see value in the build version remark for our current build system, but I do think that this should be handled on the Rundeck side, such that all three OS build jobs will get the same timestamp as build version. I'm hoping @evtrados can pick this up

jellespijker and others added 12 commits April 14, 2022 07:59
Contributes to CURA-8640
Contributes to CURA-8640
Contributes to CURA-8640
Contributes to CURA-8640
Co-authored-by: Ghostkeeper <Ghostkeeper@users.noreply.github.com>
Not needed anymore

Contributes to CURA-8640
Code style

Contributes to CURA-8640
Left over debugging

Contributes to CURA-8640
Contributes to CURA-8640
Co-authored-by: Ghostkeeper <Ghostkeeper@users.noreply.github.com>
See semver.org

Contributes to CURA-8640
Since these don't give support for C++17

Contributes to CURA-8640
@jellespijker
Copy link
Member Author

Created a ticket CURA-9157 for the MSI installer

@jellespijker
Copy link
Member Author

Removing the CMake variables for the branches is best done by @evtrados when he creates the Rundeck scripts

@jellespijker
Copy link
Member Author

See CURA-9105 for the uninstaller

Contributes to CURA-8640
@jellespijker
Copy link
Member Author

I have updated the readme with the minimal information.

I have left out the Docker use case Since that is basically just a tool for us, to have a reproducible environment.

@Ghostkeeper Ghostkeeper merged commit 107774e into 5.0 Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.