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

Introduce Conan, JFrog and GitHub Actions to build and manage Cura (and her dependencies #12708

Merged
merged 384 commits into from
Jul 13, 2022

Conversation

jellespijker
Copy link
Member

@jellespijker jellespijker commented Jul 8, 2022

Request to the reviewers.
please use the suggestions functionality or take the liberty to change things on your own accord. I trust your judgement.
Also be aware that we still have the Conan epic coming along in which we have plenty of opportunities to change things. Maybe add issues to the Conan eCCB (link to be added)

Overview

Important

  • Start merging bottom up in the dependency tree
  • Once everything (including this PR) is merged to main, go through the workflows to remove branch specific logic
  • Once everything (including this PR) is merged to main, got through the GitVersion.yml files to bump up the version to 5.2
  • Once everything (including this PR) is merged to main, go through the conandata.yml files to add the correct blocks

And then we see what breaks. Don't hesitate to contact me.

Closes issues

If you think your issue or pull request has been closed prematurely and is still relevant. Please open a new issue in this repository. To much has changed in the mean time. Best to keep discussions centralized with regard to this build system.

Also try out the new documentation first. Hopefully they will solve your issue, but if those don't cover the scenario's you guys have then please let us know so we can update these. See:

@nallath I think it would be a good idea to create a label specific to build/development.

Related issues and pull requests

[email protected] and others added 30 commits June 28, 2022 07:43
Still WIP, but needs to be committed due to not copying sources (1gb saving)

Contributes to CURA-9365
Something similar should be done for Uranium

Contributes to CURA-9365
The following options shouldn't be used to determine the hash, since these are only used to set the CuraVersion.py
which will als be generated by the deploy method during the `conan install cura/5.1.0@_/_`

Contributes to CURA-9365
It's the same hack that we already use for the other instances where we use the tableModel.
For some reason it wasn't done here, so we also had the not updating bug

CURA-9270
With the updated support settings of PP-108 some settings results in warnings. The warning limits are changed to avoid this. PP-185.
PVA is stringing a lot due to the too small retract distance. This oozed material is causing under extrusion elsewhere, reducing the strength of the support. Too long retractions could cause reliability issues as well. However, since we reduced the number of retractions by ~40% for the smaller models (for bigger models it is ~0%, but then it is also not relevant since a lot of material is printed in between) with the new settings of PP-108 increasing the retract distance to 6.5mm is safe. PP-185
A bigger horizontal expansion is improving the stability of the support structure. Setting the Support Horizontal Expansion > Support X/Y distance causes the support folding around the object, making it way harder to remove and introducing a lot of additional stringing. PP-185
Setting this value smaller could cause scarring when the nozzle offset calibration is not performed. A higher value causes a too big gap between the model and support, resulting in support not being attached to the model on sloped surfaces. PP-185
Some paths in dependencies were moved around due to the new deploy functionality
this should fix it.

Contributes to CURA-9365
Also added share/<dep>/resources releative to sys.executable

Contributes to CURA-9365
We use word-capitalisation in these setting labels. And we should put a period after sentences in the description.

Contributes to issue CURA-9432.
We already had to customize to much, it isn't worth
the effort to continue with GitVersion

The script still has some issues when a tag is pushed

Contributes to CURA-9365
Contributes to CURA-9365
This can result in a strange versioning compared
to the previous 5.1.0 beta,

but it will only be cosmetic

Contributes to CURA-9365
@jellespijker
Copy link
Member Author

I had to rewrite the reusable workflow for the semver. We needed to customize the GitVersion Action to much, so I decided to write a simple Python script.

This means however that the build metadata is slightly different then we currently use. Since this doesn't affect semver, it will purely be an aesthetic thing. The next Cura-5.1.0-beta+1234 will have a lower version for the build metadata then we had yesterday.

.github/workflows/conan-package.yml Show resolved Hide resolved

jobs:
conan-recipe-version:
uses: ultimaker/cura/.github/workflows/conan-recipe-version.yml@CURA-9365
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this to reference latest after/just before merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

After

Copy link
Member Author

@jellespijker jellespijker Jul 12, 2022

Choose a reason for hiding this comment

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

Also merge cura first,
Then you can change it before in all the other repo's

If you don't immediately delete this branch then it will still be used after the merge.

As a clean up action we should still fix afterwards.

requirements-dev.txt Show resolved Hide resolved
Copy link
Contributor

@casperlamboo casperlamboo left a comment

Choose a reason for hiding this comment

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

Changes look good from what I can see, definitely an improvement compared to the old build system and we can already benefit from the github runners and it works great!

Posted some remarks through out the code. We did have a somewhat bigger remark regarding the regex of the versioning numbering. The pattern we want to match on is usually something like x.x (where x is any digit). The regex for doing so is commonly expressed in this PR as [0-9]+.[0-9]+. The dot in regex does not mean the character dot but is a special token that matches on anything. If we want to use the character . the dot needs to be escaped.


- name: Use Conan download cache (Powershell)
if: ${{ runner.os == 'Windows' }}
run: conan config set storage.download_cache="C:\Users\runneradmin\.conan\conan_download_cache"
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
run: conan config set storage.download_cache="C:\Users\runneradmin\.conan\conan_download_cache"
run: conan config set storage.download_cache="%HOMEPATH%\.conan\conan_download_cache"

why not use %HOMEPATH% here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I ran to issues with it early on.

Try changing it, but
If anything we should use the PowerShell env naming scheme: $Env:<name>, since Windows uses the PowerShell by default


- name: Install Linux system requirements
if: ${{ runner.os == 'Linux' }}
run: sudo apt install build-essential checkinstall libegl-dev zlib1g-dev libssl-dev ninja-build autoconf libx11-dev libx11-xcb-dev libfontenc-dev libice-dev libsm-dev libxau-dev libxaw7-dev libxcomposite-dev libxcursor-dev libxdamage-dev libxdmcp-dev libxext-dev libxfixes-dev libxi-dev libxinerama-dev libxkbfile-dev libxmu-dev libxmuu-dev libxpm-dev libxrandr-dev libxrender-dev libxres-dev libxss-dev libxt-dev libxtst-dev libxv-dev libxvmc-dev libxxf86vm-dev xtrans-dev libxcb-render0-dev libxcb-render-util0-dev libxcb-xkb-dev libxcb-icccm4-dev libxcb-image0-dev libxcb-keysyms1-dev libxcb-randr0-dev libxcb-shape0-dev libxcb-sync-dev libxcb-xfixes0-dev libxcb-xinerama0-dev xkb-data libxcb-dri3-dev uuid-dev libxcb-util-dev libxkbcommon-x11-dev -y
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe these dependencies can be stored similar to the requirements.txt we have for our python dependencies. In my opinion such a file is much more maintainable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not an Ubuntu-man myself if we can store these in a system deps file, then I'm all for it. But I myself don't have a ready made solution for this in mind.

Maybe you guys can look into that

.github/workflows/conan-package-create.yml Show resolved Hide resolved
.github/workflows/conan-package-create.yml Show resolved Hide resolved
.github/workflows/conan-package.yml Outdated Show resolved Hide resolved
GitVersion.yml Outdated Show resolved Hide resolved
'CFBundleShortVersionString': {{ short_version }},
'CFBundleDocumentTypes': [{
'CFBundleTypeRole': 'Viewer',
'CFBundleTypeExtensions': ['*'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the list of extensions supported with cura. I think we should be a little more specific with the extensions. From MacOS right click menu I'm able to open for instance scad files with cura, only to be prompted with an error message saying the file format is not supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the same list as we previously used Cura 4.13.1 and lower. It just got left behind when we moved away from the cura-build to cura-build-environment I reintroduced it since there were also some GH issues with the Version set to 0.0.0

I agree with you that it is a very blunt approach, but I thought it best to use the proven settings for now.

I'm open to suggestions btw

Comment on lines +27 to +28
for i, v in enumerate(sys.argv):
print(f"{i} = {v}")
Copy link
Contributor

Choose a reason for hiding this comment

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

are these left-over debug statements? or are these used to repeat/duplicate the path variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

Imo that whole script needs a rehaul, since it takes way to many arguments and it should be setup in a similar fashion as the AppImage and DMG scripts.
This was currently the least amount of effort copy-paste from cura-build-environment

Comment on lines +53 to +70
nsis_content = template.render(
app_name = sys.argv[3],
main_app = sys.argv[4],
version_major = sys.argv[5],
version_minor = sys.argv[6],
version_patch = sys.argv[7],
version_build = sys.argv[8],
company = sys.argv[9],
web_site = sys.argv[10],
year = datetime.now().year,
cura_license_file = Path(sys.argv[11]),
compression_method = sys.argv[12], # ZLIB, BZIP2 or LZMA
cura_banner_img = Path(sys.argv[13]),
cura_icon = Path(sys.argv[14]),
mapped_out_paths = mapped_out_paths,
rmdir_paths = rmdir_paths,
destination = Path(sys.argv[15])
)
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we use python deconstructor statements here?

Suggested change
nsis_content = template.render(
app_name = sys.argv[3],
main_app = sys.argv[4],
version_major = sys.argv[5],
version_minor = sys.argv[6],
version_patch = sys.argv[7],
version_build = sys.argv[8],
company = sys.argv[9],
web_site = sys.argv[10],
year = datetime.now().year,
cura_license_file = Path(sys.argv[11]),
compression_method = sys.argv[12], # ZLIB, BZIP2 or LZMA
cura_banner_img = Path(sys.argv[13]),
cura_icon = Path(sys.argv[14]),
mapped_out_paths = mapped_out_paths,
rmdir_paths = rmdir_paths,
destination = Path(sys.argv[15])
)
_program, dist_loc, jinja_template_path, app_name, main_app, version_major, version_minor, version_patch, version_build, company, company, web_site, cura_license_file, compression_method, cura_banner_img, cura_icon, destination, icon_path, *args = sys.argv
nsis_content = template.render(
app_name = app_name,
main_app = main_app,
version_major = version_major,
version_minor = version_minor,
version_patch = version_patch,
version_build = version_build,
company = company,
web_site = web_site,
year = datetime.now().year,
cura_license_file = Path(cura_license_file),
compression_method = compression_method, # ZLIB, BZIP2 or LZMA
cura_banner_img = Path(cura_banner_img),
cura_icon = Path(cura_icon),
mapped_out_paths = mapped_out_paths,
rmdir_paths = rmdir_paths,
cura_icon = Path(cura_icon)
)

Copy link
Member Author

Choose a reason for hiding this comment

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

See previous comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment