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

chore(ci): refactor indiv jobs #432

Merged

Conversation

YOU54F
Copy link
Member

@YOU54F YOU54F commented Jun 5, 2024

Building individual targets allows us to identify where a single one fails, and gives us quicker feedback.

This mitigates some of the issues seen when building multiple targets with cross (wrong linker symbols), and allows us to enable per target caching which improves compile times.

Few other quality of life improvements are included (cron job, smoke test to weed out loading issues with the shared libraries)

Breaking changes

BREAKING CHANGE: macos aarch64 binaries now renamed to <app_name>-macos-aarch64. (previously was apple-darwin for aarch64)

Rationale: for consistency with x86_64 apple target and other platforms

BREAKING CHANGE: macos binaries are now only published with <app_name>-macos-. osx flavoured varieties are now discontinued

Rationale: for consistency with nomenclature, as osx not relevant today with latest version being macos 14

BREAKING CHANGE: sha256 files no longer contain full paths to files, only filenames

Rationale: The additional information is superfluous for end users, and requires them to modify the file for it to be usable. see pact-net

this avoids unexpected changes if built in a different location and avoids end users needing to update the file manually if the run the shasum check in the same folder.

BREAKING CHANGE: sha256 files no longer contain full paths to files, only filenames
split to allow building individual targets, or multiple

- generate musl .so with `RUSTFLAGS="-C target-feature=-crt-static"`

linux/windows only
BREAKING CHANGE: macos aarch64 binaries now renamed to <app_name>-macos-aarch64. (previously was apple-darwin for aarch64)

BREAKING CHANGE: macos binaries are now only published with <app_name>-macos-<arch>. `osx` flavoured varieties are now discontinued
- add workflow_dispatch / cron schedule once a week for early alerts
- support rust caching (individual caching keys)

individual targets allow us to avoid needing to do selecting cargo build cleaning due to cross-rs linking errors. Having a restored cache helps us speed up builds, which is always nice
@YOU54F YOU54F force-pushed the chore/ci_refactor_indiv_jobs branch from 5d4505b to cf1bed4 Compare June 8, 2024 01:51
@YOU54F YOU54F marked this pull request as ready for review June 8, 2024 22:50
@rholshausen
Copy link
Contributor

I hope you know what you are doing!

@rholshausen rholshausen merged commit 07418cc into pact-foundation:master Jun 11, 2024
37 checks passed
echo "flags=" >> "$GITHUB_OUTPUT"
fi

echo "flags=--release" >> "$GITHUB_OUTPUT"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since release builds can be significantly slower, should this still be only selectively enabled?

Copy link
Member Author

@YOU54F YOU54F Jun 11, 2024

Choose a reason for hiding this comment

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

so originally when I was able to build for multi targets with cross in a single run, but I actually run out of disk space on the gh runner because the debug build folder was so large (and also results in a hefty cache to store and retrieve).

this workflow is only for building the released artifacts, until this PR where it does the most basic of smoke tests.

the actual build ffi workflow builds and tests the ffi lib cross plat (although not all of our targets, just x86_64 win/lin/mac)

so the rationale was if this is able to smoke test a full release wf before each target is actually released (so via the PR workflow), we actually know whether it will work when we push a release tag.

Maybe we could just run debug on the pr's, but run in release mode on master, so before someone does push a release tag, they have confidence it will just work (tm)

I feel like we could actually probably consolidate some of our workflows to avoid de-depulication, and maximise our caching potential. we have a pact rust workflow which tests everything, pact ffi workflow that builds the project with cmake (which we don't use in the release workflow), and c smoke tests on linux against.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we cache the CARGO_TARGET_DIR, we would likely get most of the speed up anyway. The compiler would not need to recompile most of the dependencies in the first place.

@YOU54F
Copy link
Member Author

YOU54F commented Jun 11, 2024

I hope you know what you are doing!

😅

I've given it a good smoke test across all the projects consuming it that are in my immediate radar (pact-js-core / pact-php / pact-net / pact-ruby-ffi / pact-go )

It's certainly reduced my feedback loop and helped me diagnose single target failures, and hopefully this should mitigate some of the initial concerns we get from maintainers on a new update. it's bork and we don't know, and have to go through the release shenanigans again with a fix that may or may not work (when its something in the cross compilation build process, that is a bit of a black box without being prepared to go really deep in yak shave territory)

I write up some release notes for the release page when this drops if it doesn't pick up my breaking change footer

@YOU54F YOU54F deleted the chore/ci_refactor_indiv_jobs branch June 26, 2024 01:11
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.

0.4.20 pact_ffi - aarch64-musl .so failing to load (missing symbols)
3 participants