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

refactor(cmake): split CMakeLists into modules #1587

Merged

Conversation

ReenigneArcher
Copy link
Member

@ReenigneArcher ReenigneArcher commented Sep 1, 2023

Description

The current CMakeLists.txt file is getting pretty out of hand and overly complex. This PR aims to refactor CMakeLists into modules.

Other minor adjustments:

  • tray icon can now be disabled
  • renamed some build options (mostly around special packaging like AppImage, Flatpak, Portfile... probably shouldn't be considered a breaking change)
  • sunshine.desktop will now be configured for all Linux builds
    • 2 desktop files for flatpak due to different exec commands
    • AppImage desktop file is different due to additional keys
  • added sunshine.appdata.xml for Linux packages
  • project HOMEPAGE_URL updated
  • added PROJECT_LICENSE variable to cmake build
  • renamed PKGBUILD directory in source code
  • setup Linux powerpc64le ffmpeg options (probably need a better way to use the pre-built packages than using submodules)
  • sort supported cuda architectures in cmake message and docs
  • rename AppImage ci job to Linux AppImage

Screenshot

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

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 in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@LizardByte-bot
Copy link
Member

LizardByte-bot commented Sep 1, 2023

Qodana success
Results:
- python
- js

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Qodana for JS

It seems all right 👌

No new problems were found according to the checks applied

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/[email protected]
        with:
          upload-result: true
Dependencies licenses

Third-party software list

This page lists the third-party software dependencies used in project

Dependency Version Licenses
@fortawesome/fontawesome-free 6.4.0 OFL-1.1-no-RFN
@popperjs/core 2.11.8 MIT
bootstrap 5.3.0 MIT
vue 2.6.12 MIT
Contact Qodana team

Contact us at [email protected]

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Qodana for Python

It seems all right 👌

No new problems were found according to the checks applied

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/[email protected]
        with:
          upload-result: true
Contact Qodana team

Contact us at [email protected]

@ReenigneArcher ReenigneArcher force-pushed the refactor(cmake)-split-CMakeLists-into-modules branch 6 times, most recently from 7c5eed9 to 16175a7 Compare September 1, 2023 02:53
@ReenigneArcher ReenigneArcher mentioned this pull request Sep 1, 2023
11 tasks
@ReenigneArcher ReenigneArcher force-pushed the refactor(cmake)-split-CMakeLists-into-modules branch 13 times, most recently from efb6e33 to 212e266 Compare September 2, 2023 00:37
@ReenigneArcher ReenigneArcher marked this pull request as ready for review September 2, 2023 00:49
@ReenigneArcher
Copy link
Member Author

ReenigneArcher commented Sep 2, 2023

This is a pretty big PR. I would appreciate a review, and I apologize it's not the easiest to review since it shows almost everything as deleted and added. Most of what is shown as added, did previously exist... but there are cases where it was adjusted in one way or another.

In the end I think organizing cmake like this (or in a similar way) will be better for the project, and make it easier to understand what is happening... the existing CMakeLists.txt was also blocking me from adding unit testing, as I just wasn't able to make sense of all of it.

There might be more opportunities to re-order some things in the various .cmake files. I'm open to any suggestions.

@ReenigneArcher ReenigneArcher force-pushed the refactor(cmake)-split-CMakeLists-into-modules branch from 212e266 to 8d3f0a2 Compare September 2, 2023 22:29
@ns6089
Copy link
Contributor

ns6089 commented Sep 3, 2023

It makes sense separating platforms (and especially packaging platforms), but not a fan of splitting the files within a platform.
Now to edit let's say windows built, I need to keep track of 5 files, not counting common.

@psyke83
Copy link
Collaborator

psyke83 commented Sep 3, 2023

CMake Warning (dev) in CMakeLists.txt:
  No project() command is present.  The top-level CMakeLists.txt file must
  contain a literal, direct call to the project() command.  Add a line of
  code such as

    project(ProjectName)

  near the top of the file, but after cmake_minimum_required().

  CMake is pretending there is a "project(Project)" command on the first
  line.
This warning is for project developers.  Use -Wno-dev to suppress it.

This explains the discrepency of the CMakeCache.txt on Windows with this PR:

 //Install path prefix, prepended onto install directories.
-CMAKE_INSTALL_PREFIX:PATH=C:/Program Files (x86)/Sunshine
+CMAKE_INSTALL_PREFIX:PATH=C:/Program Files (x86)/Project

What's interesting is that neither of these variables are correct (should be C:/Program Files/Sunshine for the x64 build), but this probably doesn't matter, as it's only used by cmake's --install parameter, and the generated NSIS installer selects the correct path.

Aside from this minor issue, local builds of Windows and Ubuntu 23.04 .deb packages seem OK.

@ReenigneArcher
Copy link
Member Author

@psyke83 I'll move the base.cmake back to the main file.

@ReenigneArcher ReenigneArcher force-pushed the refactor(cmake)-split-CMakeLists-into-modules branch 4 times, most recently from 61b3ce6 to fdb116b Compare September 4, 2023 02:48
@ReenigneArcher ReenigneArcher force-pushed the refactor(cmake)-split-CMakeLists-into-modules branch from fdb116b to 251550b Compare September 4, 2023 03:08
@ReenigneArcher ReenigneArcher merged commit 92b4eee into nightly Sep 5, 2023
@ReenigneArcher ReenigneArcher deleted the refactor(cmake)-split-CMakeLists-into-modules branch September 5, 2023 00:16
@ReenigneArcher ReenigneArcher added this to the Implement Unit Testing milestone Mar 16, 2024
KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Jun 6, 2024
e-dong pushed a commit to e-dong/Sunshine that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants