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

Framework: Add Configurable Build Architectures and Input Flags #6247

Merged
merged 6 commits into from
Oct 2, 2024

Conversation

mreid-tt
Copy link
Contributor

@mreid-tt mreid-tt commented Oct 2, 2024

Description

This PR introduces several updates to the build workflow, including:

  • Added new input flags to optionally include specific architectures for SRM 1.2, DSM 5.2, DSM 6.2, DSM 7.1, and DSM 7.2 (required for Jellyfin).
  • Introduced logic to generate a dynamic matrix of architectures based on user input (noarch architectures included by default).
  • Made adjustments to conditionally include older DSM architectures, improving build flexibility and reducing unnecessary builds.

These changes optimise the build process, allowing more control over which architectures are built and improving workflow performance by excluding unused architectures.

Related to #6101

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

@mreid-tt mreid-tt requested a review from th0ma7 October 2, 2024 13:20
@mreid-tt mreid-tt self-assigned this Oct 2, 2024
@mreid-tt mreid-tt changed the title Framework: Include optional dsm 7.2 archs on build Framework: Add optional DSM 7.2 build architectures Oct 2, 2024
@mreid-tt mreid-tt requested a review from hgy59 October 2, 2024 13:24
@th0ma7
Copy link
Contributor

th0ma7 commented Oct 2, 2024

Wow i love that! Not sure how the code works out but that's great. Could you fake a test build and confirm it does work as expected and include dsm 7.2 only when needed?

@hgy59
Copy link
Contributor

hgy59 commented Oct 2, 2024

great 🔝

.github/workflows/build.yml Outdated Show resolved Hide resolved
@hgy59
Copy link
Contributor

hgy59 commented Oct 2, 2024

some suggestions

  • If dsm 7.2 builds are "exclusive or" to dsm 7.1 builds, we could make *-7.1 values optional too, with optional: ${{ github.event.inputs.include_optional_arch == 'false' }}
  • I would like to rename the option include_optional_arch to add_dsm72_builds or prefere_dsm72_builds (depending whether those are additional or exclusive builds).
  • there are two more candidates for additional builds: add_srm_builds and add_dsm52_builds (both default to false, those are not yet in the matrix and would be added with armv7-1.2, ppc853x-5.2, 88f6281-5.2 and x86-5.2)
  • we could prepare to make dsm6 builds optional with an option for add_dsm6_builds with default value true (but sooner or later it will default to false)

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

some suggestions

hey @hgy59, these were excellent additions to make the approach more extensible. I've switched to something that dynamically generates the matrix with the last commit which should allow us to do everything you suggested.

Wow i love that! Not sure how the code works out but that's great. Could you fake a test build and confirm it does work as expected and include dsm 7.2 only when needed?

hey @th0ma7, I did push this commit to my fork of the repo and the new build interface looks like this:

Screenshot 2024-10-02 at 2 43 13 PM

With the above settings the build ran successfully like this:

Screenshot 2024-10-02 at 2 42 09 PM

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

@hgy59, I've further refined the input so you have drop-down menus so as to avoid input errors:

Screenshot 2024-10-02 at 2 57 58 PM

Do let me know if I have your apptoval to merge this.

@mreid-tt mreid-tt requested review from hgy59 and removed request for th0ma7 October 2, 2024 19:01
@mreid-tt mreid-tt changed the title Framework: Add optional DSM 7.2 build architectures Framework: Add Configurable Build Architectures and Input Flags for SRM and DSM Oct 2, 2024
@mreid-tt mreid-tt changed the title Framework: Add Configurable Build Architectures and Input Flags for SRM and DSM Framework: Add Configurable Build Architectures and Input Flags Oct 2, 2024
@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

@th0ma7, once this is merged you should be able to remove the changes to the build.yml in PR #6239.

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 2, 2024

@th0ma7, once this is merged you should be able to remove the changes to the build.yml in PR #6239.

will definiteively do, probably as a standalone PR to avoid needing github-action to do all the rebuilds once more.

@mreid-tt does that only affect build run for publishing? Or is that option somehow available on a per PR basis (guessing by playing into the resulting build tab or similar?).

@mreid-tt I suggest you make it explicit that DSM-6.2 really is only DSM-6.2.4 as sourceforge toolchains got wiped out a few months ago forcing to move to this specific sub-version.

@hgy59 relatively to DSM-5.2, isn't it only ARMv5 which is being considered? Further, are the toolchain file ready to download from an alternate source?

EDIT: And I'm glad we've now found a new github-action guru ;)

@hgy59
Copy link
Contributor

hgy59 commented Oct 2, 2024

@hgy59 relatively to DSM-5.2, isn't it only ARMv5 which is being considered? Further, are the toolchain file ready to download from an alternate source?

The ppc853x-5.2, 88f6281-5.2 and x86-5.2 cover all x10 models;
I am used to push those (most cli tools) when sucessfully built (x86-5.2 might fail as it is not handable with UNSUPPORTED_ARCHS)

  • x64-5.2 models: DS1010+, RS810+, RS810RP+, DS710+
  • 88f6281-5.2 models: DS110j, DS210j, DS410j
  • ppc853x-5.2 models: DS110+, DS210+, DS410

Edit:
The alternate sources are on https://github.com/SynoCommunity/spksrc/releases/tag/toolchains%2Fdsm5.2

@hgy59
Copy link
Contributor

hgy59 commented Oct 2, 2024

@th0ma7, once this is merged you should be able to remove the changes to the build.yml in PR #6239.

unfortunately you forgot to revert the 'build.yml' changes in #6239...

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 2, 2024

unfortunately you forgot to revert the 'build.yml' changes in #6239...

@hgy59 nope, I've intentionally done in into a subsequent PR to avoid waisting cpu cycles 😸

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

@mreid-tt does that only affect build run for publishing? Or is that option somehow available on a per PR basis (guessing by playing into the resulting build tab or similar?).

@th0ma7, it appears that the extended input options are only available for manual builds. Currently, GitHub Actions automatically trigger a build whenever a PR commit is pushed, which includes DSM 6.2.4 and DSM 7.1. However, while the PR is open, you can manually trigger a build action if needed. For instance, when a new Jellyfin version is submitted as a PR in the future, it will activate the default workflow (which will not build packages). This action can be manually canceled to conserve resources, and you can then trigger a manual build on the main branch (rather than your fork) to generate DSM 7.2 packages for testing before merging.

@mreid-tt I suggest you make it explicit that DSM-6.2 really is only DSM-6.2.4 as sourceforge toolchains got wiped out a few months ago forcing to move to this specific sub-version.

Good catch, this has been corrected. (and reverted)

EDIT: And I'm glad we've now found a new github-action guru ;)

I wouldn't say guru... this was me, the source code and a conversation with ChatGPT.

@hgy59
Copy link
Contributor

hgy59 commented Oct 2, 2024

@mreid-tt I would prefer to name the dsm 6 version 6.2 and not 6.2.4 since the repository has only major.minor naming too.

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

@mreid-tt I would prefer to name the dsm 6 version 6.2 and not 6.2.4 since the repository has only major.minor naming too.

Ok, will do... (done)

@hgy59
Copy link
Contributor

hgy59 commented Oct 2, 2024

it appears that the extended input options are only available for manual builds. Currently, GitHub Actions automatically trigger a build whenever a PR commit is pushed, which includes DSM 6.2.4 and DSM 7.1.

regular workflows on master and PRs will always use the default options.

If we want to customize this for PRs, we might use specific labels to evaluate the options.

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 2, 2024

Otherwise looks good to me. I'm actually planning to give it a shot to publish from my just merged pr :)

@hgy59
Copy link
Contributor

hgy59 commented Oct 2, 2024

we might reverse the order of the options to have the least important versions at the bottom ... ?

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

we might reverse the order of the options to have the least important versions at the bottom ... ?

Done. Anything else before I merge?

Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

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

LGTM

@mreid-tt mreid-tt merged commit 10b3b72 into SynoCommunity:master Oct 2, 2024
1 check passed
@mreid-tt mreid-tt deleted the framework-optional-archs branch October 2, 2024 22:22
@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

Otherwise looks good to me. I'm actually planning to give it a shot to publish from my just merged pr :)

Enjoy! Let me know how it goes...

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 2, 2024

Builds started, lets wait & see.
BTW, I find it really interesting as I probably won't ever release a DSM6 synocli-videodriver due to old compiler and frozen versions. This allows publishing accordingly to that easily. well done mate!

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 2, 2024

Builds started, lets wait & see.

Hi, I noticed that your fork of https://github.com/th0ma7/spksrc doesn't seem to be in sync with the main branch. Are you sure you're building with the intent to publish? I see some builds running on the main branch, but I was under the impression that this wouldn't work since your API key isn't available there. Could you clarify?

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 2, 2024

Self slapping... Getting late and tired.

@th0ma7
Copy link
Contributor

th0ma7 commented Oct 3, 2024

it's now really late but found cycles to sync my repository and resume jobs from my fork. wait & see in the morning.

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 3, 2024

it's now really late but found cycles to sync my repository and resume jobs from my fork. wait & see in the morning.

Oops, I've already started jobs on my fork to publish your packages. Sorry, I should have mentioned something earlier.

@hgy59
Copy link
Contributor

hgy59 commented Oct 3, 2024

@mreid-tt it looks like the creation of the build-matrix does not work in regular build actions.
only the (not conditional) noarch builds are run
see https://github.com/SynoCommunity/spksrc/actions/runs/11159541528/job/31018125120

@hgy59
Copy link
Contributor

hgy59 commented Oct 3, 2024

and on #6250 where I made the noarch packages optional too, there is an error:
Error when evaluating 'strategy' for job 'build'. .github/workflows/build.yml (Line: 186, Col: 15): Matrix must define at least one vector

it seems that the variables for the options are not available at all:

   if [ "" == "true" ]; then
    matrix+='{"arch": "noarch-1.1"},'
    matrix+='{"arch": "noarch-3.1"},'
    matrix+='{"arch": "noarch-6.1"},'
    matrix+='{"arch": "noarch-7.0"},'
  fi

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 3, 2024

@mreid-tt it looks like the creation of the build-matrix does not work in regular build actions. only the (not conditional) noarch builds are run see https://github.com/SynoCommunity/spksrc/actions/runs/11159541528/job/31018125120

Thanks for the heads-up. I think we can patch the existing build.yml code to set default values for the matrix generation when called automatically. I'll open a new PR for this later tonight.

Is that okay or do you need to fix this sooner than that?

EDIT: Proposed changes input directly in your PR #6250

@mreid-tt mreid-tt mentioned this pull request Oct 4, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants