obs-studio icon indicating copy to clipboard operation
obs-studio copied to clipboard

CI: Split Up Windows Signing Job

Open derrod opened this issue 1 year ago • 1 comments

Description

Splits up the windows signing workflow into "package" and "updater" steps which are responsible for the singing/packaging and creation of updater files/delta updates respectively.

Still a draft as...

  1. We need to figure out how to include dual-signed game capture DLLs (old + new cert) to ease the transition for anti-cheat providers
  2. I need to finalise the bouf update, probably some cleanup as some stuff isn't really needed anymore (e.g. RSA signing, support for bsdiff/lzma patches)
  3. We will probably discuss double-zipping in the packaging step vs zipping in release creation step (maybe https://github.com/actions/download-artifact/issues/143 will have some progress)
  4. Maybe figure out if we want to upload the updater files directly to the distribution system rather than somebody having to manually do that

Motivation and Context

  • Speed up signing/installer creation step (12 mins -> 6 mins)
  • Add Windows artifacts to automatically created draft release
  • Let delta generation job run as long as it needs without blocking release creation (probably more than 15 minutes if we throw ina CEF update for good luck)

How Has This Been Tested?

Partially tested on my fork, will require follow-up test once finalised.

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • [x] My code has been run through clang-format.
  • [x] I have read the contributing document.
  • [x] My code is not on the master branch.
  • [ ] The code has been tested.
  • [x] All commit messages are properly formatted and commits squashed where appropriate.
  • [x] I have included updates to all appropriate documentation.

derrod avatar Feb 13 '24 10:02 derrod

Wouldn't it make more sense to split this up into 2 separate actions (see also https://github.com/actions/cache which maintains separate save and restore actions in subdirectories).

That way you can use bouf/package and bouf/sign separately and don't have add unnecessary inputs.

Also allows us to keep each usage scenario self-contained in a bespoke action for each without having to increase complexity of a single action.

PatTheMav avatar Feb 18 '24 03:02 PatTheMav

There's still a decent bit of overlap and I didn't want to create two separate places to keep up to date as far as third-party actions and hashes goes. There are also two steps that could be removed entirely now given that we have decided not to sign manifests on CI for the foreseeable future.

Edit: Another one could be removed if we stop double-zipping our Windows builds on CI. With the new artifacts v4 system that is no longer necessary for speed.

derrod avatar Feb 22 '24 23:02 derrod

There's still a decent bit of overlap and I didn't want to create two separate places to keep up to date as far as third-party actions and hashes goes. There are also two steps that could be removed entirely now given that we have decided not to sign manifests on CI for the foreseeable future.

Edit: Another one could be removed if we stop double-zipping our Windows builds on CI. With the new artifacts v4 system that is no longer necessary for speed.

Right, I'll hold off on further review until those refinements have landed.

PatTheMav avatar Feb 23 '24 00:02 PatTheMav

One thing that should definitely be changed is the name of the action however - every other action in the repo has a name that clearly states its intent/job (e.g. "build-obs", "run-clang-format", "steam-upload"), so this one sticks out.

I'd prefer if bouf itself had gotten a more descriptive name that would clearly communicate what it does, but I guess that ship has sailed already.. 😅

PatTheMav avatar Feb 24 '24 14:02 PatTheMav

Updated now to properly support the dual-signature feature we want to use for game capture in 30.2 while we transition to the new cert. Also cleaned up a few things and tested it to make sure the draft release is created with the installer and ZIPs properly added and hashed.

One thing that should definitely be changed is the name of the action however - every other action in the repo has a name that clearly states its intent/job (e.g. "build-obs", "run-clang-format", "steam-upload"), so this one sticks out.

By that, do you meant the name of the folder (actions/bouf) should be changed?

derrod avatar Apr 13 '24 08:04 derrod

Updated now to properly support the dual-signature feature we want to use for game capture in 30.2 while we transition to the new cert. Also cleaned up a few things and tested it to make sure the draft release is created with the installer and ZIPs properly added and hashed.

One thing that should definitely be changed is the name of the action however - every other action in the repo has a name that clearly states its intent/job (e.g. "build-obs", "run-clang-format", "steam-upload"), so this one sticks out.

By that, do you meant the name of the folder (actions/bouf) should be changed?

Yep, something like actions/windows-bundler or something would be more canonical - what does bout actually stand for?

PatTheMav avatar Apr 17 '24 23:04 PatTheMav

Yep, something like actions/windows-bundler or something would be more canonical - what does bout actually stand for?

It stands for Building OBS Updates Fast(er).

derrod avatar Apr 17 '24 23:04 derrod

Is the package step (of the workflow and the action) ever run without a consecutive update step as which makes "merging" two separate workflows and actions into the same file necessary.

They're always run consecutively, they're just split up so that the creation of the draft release is not blocked by the creation of the updater files, which can take a long time (~28 minutes). Since the action in both cases is almost identical save for two steps and a command line parameter I didn't want to split it up and have to maintain two files that are mostly the same.

derrod avatar Apr 18 '24 09:04 derrod

Is the package step (of the workflow and the action) ever run without a consecutive update step as which makes "merging" two separate workflows and actions into the same file necessary.

They're always run consecutively, they're just split up so that the creation of the draft release is not blocked by the creation of the updater files, which can take a long time (~28 minutes). Since the action in both cases is almost identical save for two steps and a command line parameter I didn't want to split it up and have to maintain two files that are mostly the same.

I'd argue that the latter is preferable (see also the code formatting actions) because it puts logical representation of workflow actions/steps before implementation (as the implementation can and should change over time). If something about the packaging action changes, it then requires ensuring that both packaging and signing will work because both are intermingled in the same "thing".

Also if updater creation takes that long, I'd just throw it into the publish workflow which is run after a release has been created (that's also when we publish to Flathub, etc.) so it should not even run as part of a push IMO.

PatTheMav avatar Apr 18 '24 14:04 PatTheMav

Previously we decided that we wanted to have the updater files ready for testing and deployment before we publish a github release, so that's a question for @RytoEX if we want to go back on that. We could then also move the Sparkle job into the publish workflow and increase the number of deltas there so we cover more than one version as well (Windows currently goes back ~2 years) and just let it take however long it takes.

derrod avatar Apr 23 '24 20:04 derrod

Previously we decided that we wanted to have the updater files ready for testing and deployment before we publish a github release, so that's a question for @RytoEX if we want to go back on that. We could then also move the Sparkle job into the publish workflow and increase the number of deltas there so we cover more than one version as well (Windows currently goes back ~2 years) and just let it take however long it takes.

I think I'm okay with experimenting with this. It would mean adjusting our overall release workflow a bit (GitHub would end up being first, with everything else following after), but I'm not strictly opposed. @PatTheMav ?

RytoEX avatar Apr 24 '24 16:04 RytoEX

Previously we decided that we wanted to have the updater files ready for testing and deployment before we publish a github release, so that's a question for @RytoEX if we want to go back on that. We could then also move the Sparkle job into the publish workflow and increase the number of deltas there so we cover more than one version as well (Windows currently goes back ~2 years) and just let it take however long it takes.

I think I'm okay with experimenting with this. It would mean adjusting our overall release workflow a bit (GitHub would end up being first, with everything else following after), but I'm not strictly opposed. @PatTheMav ?

Putting the Sparkle job in the release workflow seems beneficial to me as well.

PatTheMav avatar Apr 27 '24 15:04 PatTheMav

With that in mind I would then propose the following approach:

  • Rename the bouf action to windows-signing to have a more descriptive name
    • It will remain part of the existing reusable workflow so it can be pinned to a specific commit hash that is validated by the Google Cloud authentication mechanism
  • Make patch generation a separate action called windows-patches
    • Part of release and dispatch workflows (so it can be manually rerun if necessary)
  • (In a separate PR) Move Sparkle action to the release workflow and increase the number of deltas

This will also make way for the following changes:

  • The signing action can be optimised to only download the previous release from GCS, and then sync back its output automatically
  • The patch generation action can be updated to use a static read-only access key/secret to fetch builds via the S3-compatible GCS API
    • This will allow it to no longer require approvals as it will not have access to sensitive APIs
  • We could move the patch generation workflow to Linux runners as it will no longer require any Windows-specific features, which may help speed it up a bit more

derrod avatar Apr 27 '24 20:04 derrod

Implemented the changes outlined above, tested it on my fork as well and seems to work fine.

derrod avatar Apr 28 '24 09:04 derrod

To be honest I've not really thought about how we'll handle multi-architecture Windows at all yet. Will require some further thought about how we want to implement that in the Windows updater setup, e.g. go the Sparkle route and have different feeds entirely, or do we want something unified (no need to duplicate non-binary files), etc.

derrod avatar Apr 28 '24 10:04 derrod