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

WIP: Moving to cpack in order to unify installers across all platforms #139

Closed
wants to merge 13 commits into from

Conversation

ABeltramo
Copy link
Contributor

@ABeltramo ABeltramo commented Apr 23, 2022

Description

I'm moving package distribution to cpack which will unify the current .deb and .rpm generation and it'll enable creation of Windows Installers and Mac OSX bundles.
This should reduce considerably duplication of code.

Currently work in progress, needs testing on real deployments.

Issues Fixed or Closed

Type of Change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the documentation blocks for new or existing components

@ABeltramo
Copy link
Contributor Author

I'll modify the docs to reflect the change, if someone wants to try this out it's as simple as:

mkdir build # I like to put all the generated files inside here, so that we don't clutter the root
cd build
cmake ../
cpack -G DEB # you can also try RPM

Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Once you're happy with everything, CI will also need to be updated. Also, once the CI is updated, we can run it when you push new changes, and have downloadable artifacts from the workflow run.

Linux builds: https://github.com/SunshineStream/Sunshine/blob/c4054c75a7fdd0c336db326af5a77171db072e49/.github/workflows/CI.yml#L146

Windows build: https://github.com/SunshineStream/Sunshine/blob/c4054c75a7fdd0c336db326af5a77171db072e49/.github/workflows/CI.yml#L197

The final action in each job expects the release asset to be in the artifacts directory.

set(CMAKE_INSTALL_PREFIX "/etc/sunshine")
endif()

include(CPack)
Copy link
Member

Choose a reason for hiding this comment

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

add new line at end of files

assets/linux-deb/conffiles Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

Getting close!

Comment on lines 166 to 167
cp Sunshine__.rpm ../artifacts/
cp Sunshine__.deb ../artifacts/
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as windows regarding move.

Let's use these filenames:
sunshine.deb
sunshine.rpm

Also, while changes are being made, let's order them alphabetically. deb first, then rpm.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
Comment on lines 216 to 217
mkdir artifacts

Copy link
Member

Choose a reason for hiding this comment

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

won't need this here

Comment on lines 232 to 223
- name: Package Windows
run: |
cd sunshine-windows-build
del ..\assets\apps_linux.json
7z a sunshine-windows.zip ..\assets
7z a sunshine-windows.zip sunshine.exe
7z a sunshine-windows.zip tools\dxgi-info.exe
7z a sunshine-windows.zip tools\audio-info.exe
7z a sunshine-windows.zip tools\sunshinesvc.exe
7z a sunshine-windows.zip ..\tools\install-service.bat
7z a sunshine-windows.zip ..\tools\uninstall-service.bat
cd ..
mkdir artifacts
move "sunshine-windows-build\sunshine-windows.zip" "artifacts"
cpack

cp Sunshine__.exe ../artifacts
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the package step as we discussed. Would be good to have a portable version as well.

Probably doesn't make much difference, but I think it would be better (slightly faster) to use move... also let's rename the file to sunshine-windows-installer.exe

Probably makes most sense to have both cpack and the move in the package step.

      - name: Package Windows
        run: |
          cd sunshine-windows-build

          cpack

          del ..\assets\apps_linux.json
          7z a sunshine-windows.zip ..\assets
          7z a sunshine-windows.zip sunshine.exe
          7z a sunshine-windows.zip tools\dxgi-info.exe
          7z a sunshine-windows.zip tools\audio-info.exe
          7z a sunshine-windows.zip tools\sunshinesvc.exe
          7z a sunshine-windows.zip ..\tools\install-service.bat
          7z a sunshine-windows.zip ..\tools\uninstall-service.bat

          mkdir artifacts
          move "sunshine-windows.zip" "../artifacts"
          move "Sunshine__.exe" "../artifacts/sunshine-windows-installer.exe"

set(CPACK_PACKAGE_HOMEPAGE_URL "https://github.com/SunshineStream/Sunshine")
set(CPACK_RESOURCE_FILE_LICENSE ${PROJECT_SOURCE_DIR}/LICENSE)
set(CPACK_PACKAGE_ICON ${PROJECT_SOURCE_DIR}/sunshine.png)
set(CPACK_PACKAGE_FILE_NAME "${CMAKE_PROJECT_NAME}_${VERSION}_${CPACK_DEBIAN_PACKAGE_ARCHITECTURE}")
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this needs to change to prevent this (same with start menu entry).

image

CMakeLists.txt Outdated
set(CPACK_PACKAGE_CONTACT "https://github.com/SunshineStream/Sunshine")
set(CPACK_DEBIAN_PACKAGE_MAINTAINER "https://github.com/SunshineStream/Sunshine")
set(CPACK_PACKAGE_DESCRIPTION "Gamestream host for Moonlight")
set(CPACK_PACKAGE_HOMEPAGE_URL "https://github.com/SunshineStream/Sunshine")
Copy link
Member

Choose a reason for hiding this comment

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

https://sunshinestream.github.io

Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

There might be more, but this is what I found so far.

Comment on lines 235 to 272
del ..\assets\apps_linux.json
7z a sunshine-windows.zip ..\assets
7z a sunshine-windows.zip sunshine.exe
7z a sunshine-windows.zip tools\dxgi-info.exe
7z a sunshine-windows.zip tools\audio-info.exe
7z a sunshine-windows.zip tools\sunshinesvc.exe
7z a sunshine-windows.zip ..\tools\install-service.bat
7z a sunshine-windows.zip ..\tools\uninstall-service.bat
cd ..
mkdir artifacts
move "sunshine-windows-build\sunshine-windows.zip" "artifacts"
- name: Upload Artifacts
cpack

mkdir -p artifacts/standalone
mkdir -p artifacts/web
mkdir -p artifacts/shaders/directx

# Installers
mv Sunshine__.exe artifacts/sunshine-windows-installer.exe

# Standalone
mv ../assets/web artifacts/web
mv tools artifacts/tools
mv ../assets/shaders/directx artifacts/shaders/directx
mv ../assets/apps_windows.json artifacts/apps_windows.json
mv ../assets/sunshine.conf artifacts/sunshine.conf
mv sunshine.exe artifacts/sunshine.exe
Copy link
Member

Choose a reason for hiding this comment

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

This won't work like this. We need only 2 files in the artifacts folders.

  • installer exe
  • zip (portable version as before)

This is because every file single in the assets directory will be uploaded as a release asset.

My code snip before should work (except the mkdir artifacts needed to be moved)

Copy link
Member

Choose a reason for hiding this comment

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

This should be pretty close #139 (comment)

Comment on lines 273 to 258
- name: Upload Sunshine Installer
if: ${{ github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' }}
uses: actions/upload-artifact@v3
with:
name: sunshine-windows-installer
path: artifacts/sunshine-windows-installer.exe
Copy link
Member

Choose a reason for hiding this comment

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

This won't be necessary because just using path: artifacts will upload all the files that exist in that folder (this will be the same behavior for the create release action)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with this is that Github will make one .zip file that contains both the .exe installer and the .zip with the sunshine standalone package resulting in a 20MB zip to be donwloaded that basically contains everything twice just packaged in two different formats (see: https://github.com/ABeltramo/Sunshine/actions/runs/2249492914).

I think it's better to split them in two separate 10MB artifacts so that users can download only the one needed.

Copy link
Member

Choose a reason for hiding this comment

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

It only does it for artifacts though, not releases. Users are rarely downloading artifacts.

Just trying to keep the CI more simple as it's already overly complex.

Comment on lines 279 to 284
- name: Upload Sunshine executable
if: ${{ github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch' }}
uses: actions/upload-artifact@v3
with:
name: sunshine-windows
path: artifacts/
name: sunshine-windows-standalone
path: artifacts/standalone
Copy link
Member

Choose a reason for hiding this comment

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

Revert this

@@ -32,7 +32,6 @@ jobs:
check_versions:
name: Check Versions
runs-on: ubuntu-latest
needs: check_changelog
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed? It's required here.

Check changelog runs first (doesn't run except on PRs or Pulls into master branch). Makes sure that the version in the changelog was updated and is not equal to the latest released version.

Then check versions runs... makes sure that the version number in the CMakeLists.txt was updated.

.github/workflows/CI.yml Show resolved Hide resolved
@ABeltramo
Copy link
Contributor Author

ABeltramo commented Apr 30, 2022

Current status:

  • rebased on top of latest nightly
  • fixed Github Actions to generate all packages, see: latest action
    • Windows installer and standalone zip should be good to go
    • Linux .deb and .rpm needs to be tested, appimage should be unchanged (unfortunately it's not supported by cpack)
    • OSX bundle .dmg can be generated but I can't run Sunshine on my Macbook M1 (segfault), also executing a .app, while possible, feels odd because it doesn't open up a terminal automatically like on Windows.
  • Generated packages should be tested on different platforms, volunteers?

This could be reviewed and merged if you are in a rush to release this, but I would like to cleanup scripts/ to have one single docker container that can build .deb and .rpm so that users can still compile without having to install extra packages on the host.

There's also probably some documentation to be updated, I haven't looked into it yet.

@ReenigneArcher ReenigneArcher mentioned this pull request Apr 30, 2022
5 tasks
@ABeltramo ABeltramo closed this May 1, 2022
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.

None yet

2 participants