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

[CI] Dry x86-64-v3 builds/default images to use glibc #4460

Merged
merged 11 commits into from Mar 11, 2024
Merged

Conversation

wileyj
Copy link
Contributor

@wileyj wileyj commented Feb 29, 2024

ref:
#4364
#4383

This adjusts the changes introduced by @jbencin that were merged in 4364 for jemalloc, and adding an additional cpu arch x86-64-v3.

  • removes the duplicated workflow to build x86-64-v3 binaries (current workflow was DRY'ed to build them via a matrix with some specific excludes)
  • fixes the dockerfiles currently in ./build-scripts to set the RUSTFLAGS env var if TARGET_CPU build-arg is provided
  • adjusts the conditional compilation flags in several files to exclude the target_os (required when cross-compiling - target_env is only application when building on a specific target triple. for github actions, the excude of target_env = msvc will not match to true since default is ubuntu vm).
  • changes the default release docker image to use debian glibc as opposed to alpine (alpine is still built, but is tagged as <release version>-alpine
  • builds the docker source image (which is triggered on any PR) with TARGET_CPU provided as x86-64-v3
  • release docker images are changed to support the following cpu types: x86-64-v3, arm64, armv7. if desired, i think we can add support for x86-64 alongside x86-64-v3

sample actions runs from my fork (testing these chagnes on a release):
https://github.com/wileyj/stacks-blockchain/actions/runs/8087234345/job/22100013368
release: https://github.com/wileyj/stacks-blockchain/releases/tag/3.0.0.0-rc1
images:
debian release image
alpine release image

sample action for the source docker image:
https://github.com/wileyj/stacks-blockchain/actions/runs/8101666085/job/22142408056
resulting image:
debian x86-64-v3 image from source

@wileyj wileyj added enhancement Iterations on existing features or infrastructure. CI benchmarking labels Feb 29, 2024
@wileyj wileyj requested review from jbencin, CharlieC3 and a team February 29, 2024 20:58
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.43%. Comparing base (04e5eb2) to head (14c3949).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4460      +/-   ##
==========================================
- Coverage   83.23%   77.43%   -5.81%     
==========================================
  Files         451      451              
  Lines      325724   325724              
  Branches      323      323              
==========================================
- Hits       271111   252216   -18895     
- Misses      54605    73500   +18895     
  Partials        8        8              
Files Coverage Δ
stackslib/src/main.rs 0.06% <ø> (ø)
testnet/stacks-node/src/main.rs 0.26% <ø> (ø)

... and 166 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04e5eb2...14c3949. Read the comment docs.

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Looks good, there was just one comment that showed up in a bunch of docker files that I don't think was intended.

build-scripts/Dockerfile.linux-glibc-arm64 Outdated Show resolved Hide resolved
build-scripts/Dockerfile.linux-glibc-armv7 Outdated Show resolved Hide resolved
build-scripts/Dockerfile.linux-musl-arm64 Outdated Show resolved Hide resolved
build-scripts/Dockerfile.linux-musl-armv7 Outdated Show resolved Hide resolved
build-scripts/Dockerfile.macos-arm64 Outdated Show resolved Hide resolved
build-scripts/Dockerfile.macos-x64 Outdated Show resolved Hide resolved
build-scripts/Dockerfile.windows-x64 Outdated Show resolved Hide resolved
jbencin
jbencin previously approved these changes Mar 1, 2024
Copy link
Contributor

@jbencin jbencin left a comment

Choose a reason for hiding this comment

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

Looks good, it's an improvement over what I had done

.github/actions/dockerfiles/Dockerfile.alpine-binary Outdated Show resolved Hide resolved
.github/actions/dockerfiles/Dockerfile.debian-binary Outdated Show resolved Hide resolved
stackslib/src/main.rs Show resolved Hide resolved
build-scripts/Dockerfile.linux-musl-x64 Outdated Show resolved Hide resolved
@wileyj
Copy link
Contributor Author

wileyj commented Mar 2, 2024

@jbencin @obycode i could also use some help undertanding the conflict i have here in clarity/Cargo.toml:

<<<<<<< ci/dry_x64_builds
[target.'cfg(all(target_arch = "x86_64", not(any(target_os="windows"))))'.dependencies]
sha2-asm = "0.5.3"

=======
>>>>>>> next

is this dependency being removed entirely?

@obycode
Copy link
Contributor

obycode commented Mar 4, 2024

Right. Jeff was able to remove that dependency.

obycode
obycode previously approved these changes Mar 4, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

lgtm!

zone117x
zone117x previously approved these changes Mar 4, 2024
Copy link
Member

@zone117x zone117x left a comment

Choose a reason for hiding this comment

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

LGTM. Kill x86-64-(legacy) imo -- can always add it back later if there is demand

@jbencin
Copy link
Contributor

jbencin commented Mar 4, 2024

is this dependency being removed entirely?

Right. Jeff was able to remove that dependency.

See #4435, which has been merged. sha2-asm is used by sha2 and doesn't need to be included directly

CharlieC3
CharlieC3 previously approved these changes Mar 4, 2024
Copy link
Member

@CharlieC3 CharlieC3 left a comment

Choose a reason for hiding this comment

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

Great job on this! :shipit:

@wileyj
Copy link
Contributor Author

wileyj commented Mar 4, 2024

LGTM. Kill x86-64-(legacy) imo -- can always add it back later if there is demand

that's doable, but it raises an interesting question around naming:
currently i'm naming v3 compiled binaries as linux-glibc-x64-v3.zip

and the legacy build as linux-glibc-x64.zip

if we were to no longer build linux-glibc-x64.zip, i'd argue we should rename the v3 build as linux-glibc-x64.zip etc.

low chances that someone is trying to run this binary on a chip that is ~10 years old, i think defaulting to v3 is the right call here.

@wileyj wileyj dismissed stale reviews from CharlieC3, zone117x, and obycode via 5ca35e8 March 4, 2024 18:59
zone117x
zone117x previously approved these changes Mar 5, 2024
CharlieC3
CharlieC3 previously approved these changes Mar 5, 2024
@wileyj wileyj dismissed stale reviews from CharlieC3 and zone117x via 11dd4d1 March 5, 2024 20:24
@wileyj
Copy link
Contributor Author

wileyj commented Mar 5, 2024

LGTM. Kill x86-64-(legacy) imo -- can always add it back later if there is demand

killed. we'll now build the same number of binaries/images but x64 builds will default to v3.
i've added enough logic in the build steps too, so we can add in v2 or later variants as needed

@wileyj
Copy link
Contributor Author

wileyj commented Mar 11, 2024

ping for second approval

@wileyj wileyj added this pull request to the merge queue Mar 11, 2024
Merged via the queue into next with commit 067633d Mar 11, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarking CI enhancement Iterations on existing features or infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants