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

Some flatpak fixing #5510

Merged
merged 23 commits into from
Jul 10, 2024
Merged

Some flatpak fixing #5510

merged 23 commits into from
Jul 10, 2024

Conversation

powpingdone
Copy link
Contributor

This is a smaller PR to attempt to address future flathub reviews. As of May 30, 1700 CST, I haven't filed the issue to flathub yet, but it will happen in the future. This is just heading off future issues.

@SoftFever
Copy link
Owner

@powpingdone

Looks the flatpak build is broken for a while.
Do you have any idea what's wrong?

@powpingdone
Copy link
Contributor Author

@SoftFever possibly that open cascade isn't being downloaded during the flatpak build

@powpingdone
Copy link
Contributor Author

@SoftFever no wait, it's running out of space again

@cochcoder
Copy link
Contributor

@SoftFever no wait, it's running out of space again

Does that also equate to the error I'm getting: Build failed: Error: The process '/usr/bin/xvfb-run' failed with exit code 1? I've noticed that the flatpak has the best chance to build on the first GitHub action when I start an entirely new PR.

@powpingdone
Copy link
Contributor Author

@cochcoder yes

@powpingdone
Copy link
Contributor Author

though, to be more specific, the xvfb-run executable is just a general running thing

@cochcoder
Copy link
Contributor

I see, could it be a issue where the flatpak isn't deleting enough stuff for its second, and so on, runs?

@powpingdone
Copy link
Contributor Author

It's possibly the CCACHE.

@cochcoder
Copy link
Contributor

Hopefully its that, it would suck if it means no more ~20 minute builds, but its much better to have a flatpak rather than nothing. Currently having a flapak version is the only way for me, and others, to run Orca Slicer reliably.

@powpingdone
Copy link
Contributor Author

powpingdone commented Jun 5, 2024

Yeah coch, I really, really, really don't like the appimage. I just wish I had more time to work on this.

@cochcoder
Copy link
Contributor

You've already done incredible work so far with the initial support. Even if this doesn't get worked on for a while, at least the majority, and I imagine the hardest, part is done.

@powpingdone powpingdone marked this pull request as ready for review June 6, 2024 05:40
@powpingdone
Copy link
Contributor Author

This should be good now that caching is disabled. THOUGH, this means that the flatpak will take significantly longer to build.

@powpingdone
Copy link
Contributor Author

@SoftFever Changes:

  • Disabled CI caching for the Flatpak
  • Included some missing includes to allow compilation
  • Fixed the commit between orcaslicer and it's fork of wx, for consistency in building.

@powpingdone
Copy link
Contributor Author

powpingdone commented Jun 17, 2024

...A few more changes, apparently: @SoftFever

  • OCCT was building Doxygen docs. A constexpr for whatever reason caused a compile error, so just disabling it outright fixed it and probably reduced the size of the build too.
  • Another missing include.
  • The base image got updated, which (I think) removed a few python dependencies for the orcaslicer init script, so I added them back.

@powpingdone
Copy link
Contributor Author

@cochcoder upstream change

@cochcoder
Copy link
Contributor

@cochcoder tl;dr: getting a blessing (api keys) from flathub

Even to upload to GitHub actions as an artifact?

@powpingdone
Copy link
Contributor Author

No, that should just be in the CI job.

@cochcoder
Copy link
Contributor

cochcoder commented Jun 18, 2024

No, that should just be in the CI job.

Strange, maybe they're using the same name...

Edit: Seems that's it

@AlessioMagni
Copy link

i'm trying to build it in fedora silverblue 40 and encountering this error

error: metadata-generation-failed

_× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.
Error: module python-setuptools_scm: Child process exited with code 1_

i'm really new to this things so i probably did something wrong, if not hope that helps

@powpingdone
Copy link
Contributor Author

@AlessioMagni make sure you're using this branch and building with it. I applied a fix for that in here.

@cochcoder
Copy link
Contributor

Thought that I would put this out there in case this information is helpful in the future. When I enable caching I find that it gets stuck on adding printer covers in ~20 mins and then silently crashes before saying it succeeded with no artifact.

@powpingdone
Copy link
Contributor Author

@SoftFever bump

@powpingdone powpingdone changed the title Some smaller flatpak improvements Some flatpak fixing Jun 30, 2024
@cochcoder
Copy link
Contributor

cochcoder commented Jul 1, 2024

I've done a few builds recently and found that the flatpak build was consistently getting this error and stopping:

make[1]: *** [CMakeFiles/Makefile2:1204: src/slic3r/CMakeFiles/libslic3r_gui.dir/all] Error 2
make: *** [Makefile:166: all] Error 2
Error: module OrcaSlicer: Child process exited with code 2
Error: Build failed: Error: The process '/usr/bin/xvfb-run' failed with exit code 1

@powpingdone
Copy link
Contributor Author

You'll need to ctrl-f "error:" for the first indication of an error.

@@ -26,6 +26,7 @@ orcaslicer_add_cmake_project(
wxWidgets
GIT_REPOSITORY "https://github.com/SoftFever/Orca-deps-wxWidgets"
GIT_SHALLOW ON
GIT_TAG a675dbe8fd07cb5c982cdab7b93cffc9d9f05f22
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to specify this since the a675dbe8fd07cb5c982cdab7b93cffc9d9f05f22 is the latest commit in master branch?

@@ -144,7 +160,7 @@ modules:
sources:
- type: git
url: https://github.com/SoftFever/Orca-deps-wxWidgets
branch: master
commit: a675dbe8fd07cb5c982cdab7b93cffc9d9f05f22
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to specify this since the a675dbe8fd07cb5c982cdab7b93cffc9d9f05f22 is the latest commit in master branch?

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, because they should always be the same and never mismatch during building both the appimage and flatpak. Really, it's a sanity check because of CI restrictions.

Copy link
Owner

Choose a reason for hiding this comment

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

Isn't it the same if we use last master branch without pining the commit?
in case we made changes in https://github.com/SoftFever/Orca-deps-wxWidgets in the future, we don't have to modify multiple places to update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it would normally, one of the things that a flathub upload would very likely require is dependency locking per release version. If a flatpak only bug is discovered, then you want to fix it without any unexpected or unneeded updates. Since this is the only dependency that has to be synchronized in a different way compared to all the other ones (due to CI size constraints), I figured this is the least intrusive method to this.

Copy link
Owner

@SoftFever SoftFever Jul 1, 2024

Choose a reason for hiding this comment

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

If a flatpak only bug is discovered, then you want to fix it without any unexpected or unneeded updates.

This is actually the opposite for third-party cases in my experience. It's better to avoid pointing to different commits for different build systems. The reason I'm asking to remove the commit tag is that the Orca-deps-wxWidgets repository is specifically created for OrcaSlicer. Any build break errors caused by the changes made in that repo should be promptly caught by the CI/CD pipeline.

If Flatpak points to an old commit, it will miss that check. The worst-case scenario is however that it may result in different behaviors compared to other platforms due to different versions of wxWidgets being used, which would be a nightmare to troubleshoot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, then you may revert both tagged parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted both tagged parts.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for the slow response.
Was a bit busy in the past two weeks.

Looks good, thank you so much!

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

Looks good
Thank you so much for the continuous effort to bring Flatpak to life!

@SoftFever SoftFever merged commit 6579549 into SoftFever:main Jul 10, 2024
16 checks passed
SoftFever added a commit that referenced this pull request Jul 11, 2024
@SoftFever
Copy link
Owner

@powpingdone
I noticed it causes build breaks: https://github.com/SoftFever/OrcaSlicer/actions/runs/9877669432/job/27279875122
I've temporarily reverted the change. Could you take a look when convenient?

@powpingdone powpingdone mentioned this pull request Jul 12, 2024
SoftFever added a commit that referenced this pull request Jul 16, 2024
SoftFever added a commit that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants