Conversation
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
Thank you for the PR, however I don't really think this PR is acceptable as is.
On Fedora and derivatives there hasn't been a stable build available for some time.
This PR won't solve that. We only create a build in our stable copr, when we create a stable release here. It's automated. And I'm not going to align Sunshine releases with Fedora releases, the same as I'm not going to align them with macOS releases or any other Linux distro.
Usually when a new Fedora version ships it requires changes to make it work with Sunshine. Often it's due to the gcc and cuda compatibility, but sometimes there are other issues because Fedora likes to ship a bleeding edge gcc version, which sometimes Sunshine is not yet able to be compiled with.
python*: no idea why these are here.
Required to generate glad sources, we no longer keep glad sources in our repo directly.
Runtime dependencies - These are all removed.
This will break rpms we pull from copr and add to GitHub releases, or downloaded manually in a PR test.
Not all tests on all targets will run successfully.
They work correctly right now. We test every PR in copr.
-Werror
Can't be disabled, because then we can no longer easily find errors in our CI builds. We like to catch problems early and get ahead of them.
Coverage - This will not work because of mixing compilers:
You have to use the same version of gcov that shipped with the compiler. Though we're not using coverage from copr build yet. Maybe we will in the future.
Cuda patches - Removed cuda patching, I assume these where cause build errors?
Required, depending of version of cuda used. I think it fixes a problem for aarch64, if my memory is correct. Might only be a problem when using -Werror.
Conclusion
I think some of what you're trying to achieve could be good to incorporate, but the way this PR goes about it is too aggressive. Maybe we can address one issue at a time in separate PRs?
Also, this should probably wait until I merge #4907 which has quite a few changes to the spec as well (though mostly dependencies).
That a shame, however that is just what this PR solves. It solves the gcc cuda compatibility by only using the compatible cuda compiler when compiling cuda files. It's basically the same as compiling the cuda parts as a separate library. Sunshine itself is compiled and linked with the host An automated trigger for a stable build for a new target would be great though.
No they will not. They are exactly the same, except for dependencies that are not needed. The required dependencies will match the target. If Sunshine needs extra dependencies (that are not linked) they can be added. This ensures that copr/rpmbuild/cmake can figure out the different runtime dependencies for all targets.
As I said, they only fail in stable (and only on some test suite creation thingy, idk), which can be expected. Also imo they are not needed for package builds.
Again this is only for the stable builds. I would suggest to allow in stable builds for a new target because the build is already well tested, patch it, or maintane a stable branch.
I know, also my suggestion that this not needed for package builds.
No we cannot. This all depends on each other and I'm not gonna figure out what tiny steps works or not. ConclusionIt would be great to get stable builds on COPR for new targets from LizardByte. This PR will fix a lot of the pain points. Imo having a stable build in COPR but then not maintain it for new targets kinda defeats the purpose of having a stable COPR repo. This PR is open for changes to make it work. |
ReenigneArcher
force-pushed
the
update-copr-builds
branch
from
21a4475 to
7aca16a
Compare
|
Need to rebase your branch, and this should be resolved. #4899 (Note: I rebased your PR)
It does not. For example you could move the dependency simplification to one PR. CUDA changes to another, etc. But if you're not willing to adjust things, that's fine. This PR doesn't solve the immediate issue you have though, which is that there are no stable release of Sunshine in https://copr.fedorainfracloud.org/coprs/lizardbyte/stable/ ... that will be resolved when I create the next Sunshine release, since in https://copr.fedorainfracloud.org/coprs/lizardbyte/beta/ we're building everything, including Rawhide. Anyway, I'll run the workflows and see where the gaps are that need to be fixed before this can be considered. |
| BuildRequires: python3-jinja2 | ||
| BuildRequires: python3-setuptools |
There was a problem hiding this comment.
Restore so we don't need to use pip to install
-- glad: using Python interpreter: /usr/bin/python3.14
-- Found Python: /usr/bin/python3.14 (found version "3.14.3") found components: Interpreter
-- glad: jinja2 or setuptools not found in /usr/bin/python3.14, installing via pip...
-- glad: pip not found in /usr/bin/python3.14, bootstrapping via ensurepip...
| BuildRequires: python311 | ||
| BuildRequires: python311-Jinja2 | ||
| BuildRequires: python311-setuptools |
| # Cross build issues | ||
| %undefine _hardened_build | ||
|
|
||
| # Define _metainfodir for OpenSUSE if not already defined |
There was a problem hiding this comment.
I prefer this comment over # Cross build issues
| fi | ||
| tar -xjf /tmp/micromamba.tar.bz2 -C /tmp | ||
| install -Dm755 /tmp/bin/micromamba %{bindir}/micromamba | ||
| micromamba create -y -p %{cudadir} cuda-nvcc |
There was a problem hiding this comment.
Is it possible to control the cuda compiler version?
This is important because it determines which GPUs are compatible with Sunshine.
| "-DBUILD_TESTS=ON" | ||
| "-DBUILD_WERROR=OFF" |
There was a problem hiding this comment.
| "-DBUILD_TESTS=ON" | |
| "-DBUILD_WERROR=OFF" | |
| "-DBUILD_WERROR=ON" |
Tests are on by default. Need WERROR ON so we can detect problems in CI, or a way to set this dynamically. e.g. For our pulls copr it needs to be ON, but can be OFF otherwise.
| # Common runtime requirements | ||
| Requires: miniupnpc >= 2.2.4 | ||
| Requires: which >= 2.21 | ||
|
|
||
| %if 0%{?fedora} | ||
| # Fedora runtime requirements | ||
| Requires: libayatana-appindicator3 >= 0.5.3 | ||
| Requires: libcap >= 2.22 | ||
| Requires: libcurl >= 7.0 | ||
| Requires: libdrm > 2.4.97 | ||
| Requires: libevdev >= 1.5.6 | ||
| Requires: libopusenc >= 0.2.1 | ||
| Requires: libva >= 2.14.0 | ||
| Requires: libwayland-client >= 1.20.0 | ||
| Requires: libX11 >= 1.7.3.1 | ||
| Requires: numactl-libs >= 2.0.14 | ||
| Requires: openssl >= 3.0.2 | ||
| Requires: pulseaudio-libs >= 10.0 | ||
| %endif | ||
|
|
||
| %if 0%{?suse_version} | ||
| # OpenSUSE runtime requirements | ||
| Requires: libappindicator3-1 | ||
| Requires: libcap2 | ||
| Requires: libcurl4 | ||
| Requires: libdrm2 | ||
| Requires: libevdev2 | ||
| Requires: libopusenc0 | ||
| Requires: libva2 | ||
| Requires: libwayland-client0 | ||
| Requires: libX11-6 | ||
| Requires: libnuma1 | ||
| Requires: libopenssl3 | ||
| Requires: libpulse0 | ||
| %endif |
There was a problem hiding this comment.
Runtime dependencies - These are all removed.
This will break rpms we pull from copr and add to GitHub releases, or downloaded manually in a PR test.
No they will not. They are exactly the same, except for dependencies that are not needed. The required dependencies will match the target. If Sunshine needs extra dependencies (that are not linked) they can be added. This ensures that copr/rpmbuild/cmake can figure out the different runtime dependencies for all targets.
I'm confused by this. We don't static link on Linux (with a few exceptions), so how will the rpm know what is a runtime dependency?
Note: The rpm depends from CMake are not carried over to this spec. That is for cpack which is not used here.
| BuildRequires: wget | ||
| BuildRequires: which | ||
|
|
||
| # For tests ⤵ |
There was a problem hiding this comment.
| # For tests ⤵ | |
| # For tests |
| "-DSUNSHINE_PUBLISHER_NAME=LizardByte" | ||
| "-DSUNSHINE_PUBLISHER_WEBSITE=https://app.lizardbyte.dev" | ||
| "-DSUNSHINE_PUBLISHER_ISSUE_URL=https://app.lizardbyte.dev/support" | ||
| "-DSUNSHINE_ENABLE_CUDA=ON" |
There was a problem hiding this comment.
| "-DSUNSHINE_ENABLE_CUDA=ON" |
It's ON by default.
| cmake_args+=("-DSUNSHINE_ENABLE_CUDA=OFF") | ||
| fi | ||
|
|
||
| # Install and setup NVM for Fedora 44+ |
There was a problem hiding this comment.
Should probably leave the nvm logic in place. This was likely a fix implemented when 44 was Rawhide. Sunshine isn't always automatically compatible with the newest node versions.
| cd %{_builddir}/Sunshine/build | ||
| %make_install | ||
| cd %{sourcedir}/build/ | ||
| xvfb-run ./tests/test_sunshine || true |
There was a problem hiding this comment.
| xvfb-run ./tests/test_sunshine || true | |
| xvfb-run ./tests/test_sunshine |
Definitely cannot silently fail here.
2 participants
Description
Hey @ReenigneArcher, I've been looking into COPR for stable builds and would like suggest these changes. These changes should allow for any Fedora build in COPR. I've tested all changes in a test copr: https://copr.fedorainfracloud.org/coprs/pvermeer/sunshine-test/monitor/
Please take a look at it. It would be great to have stable builds again on COPR.
Updated COPR spec-file so all builds from stable to latest can be build on all targets.
On Fedora and derivatives there hasn't been a stable build available for some time. The latest beta releases were causing some issues on Bazzite so I started investigating the reason and came to the conclusion that the cuda compilation is depended on the c compiler version. After inspecting this spec file I found that there is lot going on with switching compilers for targets. Fedora however does not support this very well and only provides a previous compiler and that's it.
This PR includes a different strategy for compiling Sunshine on different targets. It's using the host compiler for building and linking Sunshine except for the cuda files. The cuda files are compiled with recent and compatible compilers provided by conda. Conda is available on Fedora as an official package and I would recommend using this but openSuse Leap does not provide this package. Therefore conda is provided by micromamba which is a lean implementation of the conda package manager.
The cuda compilers
nvccand compatible c compilers are now installed via cuda-nvcc for all build targets. They are passed to cmake for compilation of the cuda files. This also a much leaner package then the package from nvidia.So basically instead changing the host compilers to build Sunshine with cuda, only the cuda files are compiled with a compatible compiler and then linked with Sunshine binary. This setup increases compatibility with all COPR targets.
Changes
Build dependencies
Runtime dependencies
These are all removed.
Copr (and cmake) can perfectly fine figure out the runtime dependencies from linking. No need to specify them unless your using a dependency directly (e.g. calling git as a command) and thus is not linked. If something is missing I can add it.
Testing
Not all tests on all targets will run successfully. I'm not sure if this can be fixed. My recommendation is to run them in a controlled environment. What I can tell these are mostly unit-tests and imo don't need te be run when building packages in COPR. I have them enabled so logs are produced in case of issues.
What I do advise to test is the generated output files and this already mostly done with validators.
Coverage
This will not work because of mixing compilers:
Version mismatch - expected 15.2 (release) (B52*) got 15.2 (experimental) (B52 )I'm not sure if this can be fixed.
Test suites
Success on current Fedora.
Fail on older targets.
Also not sure if this can be fixed.
-Werror
Disabled because stable builds will trigger some warnings to errors. This can be changed to a conditional to only disable on stable builds. Or fix/allow the errors on future stable builds.
Cuda patches
Removed cuda patching, I assume these where cause build errors?
Post script
Simplify modprobe and udev commands. No need to check for rpm-ostree and udevadm, just silently fail. Output is not displayed to the user anyways. Fedora has udevadm by default.
Misc
Build notes
Sunshine/src/nvenc/nvenc_base.cpp
Lines 23 to 25 in 1ee9163
gitas a command. It failed and then downloaded the latest version that changed a lot of symbols. I usually delete.gitwhen the source is ready and that was causing the issue. Downloading the source package from the GitHub releases would have the same issue.CI
I don't know your backend but I keep the packages in my test copr updated with my own tooling and a custom script adapted to this spec-file and my tooling.
Check for forked packages
Before a major Fedora update, COPR will branch a new build chroot from rawhide and fork the package to the new version chroot if
Follow Fedora branchingis enabled. I've found that this can be detected by getting the latest successful build id and then checking the api (build/built-packages) for chroots that now have a empty array on.packages(e.g. no build ran for this target).See: https://github.com/PVermeer/rpm-tools/blob/b1ee6501973a103b5749a8d995280d3dcc3ac8be/scripts/copr.sh#L56-L83
Tested:
I don't have ARM or nVidia hardware so this would need te be tested.
Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage