stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

feat: move the creation of source binaries to composite

Open BowTiedDevOps opened this issue 1 year ago • 7 comments

Description

I have moved the creation of the source binaries for release (along with the build-scripts directory) to the actions repository (https://github.com/stacks-network/actions/tree/main/stacks-core/create-source-binary). The change is useful because every time a change has to be made to the workflow (or the dockerfiles behind it), there's only a branch that needs to be changed instead of more, and any future changes are made into the actions repository instead of stacks-core.

BowTiedDevOps avatar Feb 12 '24 19:02 BowTiedDevOps

@BowTiedDevOps since this workflow will only be called on a release, can you share an existing action in your fork where this has run successfully?

wileyj avatar Feb 12 '24 19:02 wileyj

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (2c2afbc) 82.33% compared to head (5c6817c) 82.36%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4368      +/-   ##
===========================================
+ Coverage    82.33%   82.36%   +0.02%     
===========================================
  Files          404      404              
  Lines       292642   293057     +415     
===========================================
+ Hits        240958   241368     +410     
- Misses       51684    51689       +5     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 12 '24 19:02 codecov[bot]

a few test failures (no impact from the changes here) - the important workflow to consider is the Create Release workflow, which is using the proposed change in this PR:

https://github.com/wileyj/stacks-blockchain/actions/runs/7878574059/job/21527022696

wileyj avatar Feb 13 '24 15:02 wileyj

I'll also link an action where this workflow runs successfully. I have the same results as Jesse, with the Create Release job completing and some test failures (which are not affected by this PR). https://github.com/BowTiedDevOps/stacks-core/actions/runs/7891045810

Edit: I've also committed the checksum step fix mentioned by Jesse in his PR (https://github.com/stacks-network/stacks-core/pull/4374) in order to also have the change in develop.

BowTiedDevOps avatar Feb 14 '24 13:02 BowTiedDevOps

I'm not sure this is an improvement. The action and Dockerfiles are tightly coupled to the stacks-core repo, which has a few consequences:

  • There's no benefit to being able to reuse this action in other projects
  • If all stacks-core branches share the same action, this would make refactoring stacks-core difficult, because the Dockerfiles contains build flags and sub-project names that could change
  • Making a change to how releases are built could easily require 2 PRs, one in stacks-core and one in actions

IMO it makes more sense for these files to live in stacks-core

jbencin avatar Feb 14 '24 15:02 jbencin

I'm not sure this is an improvement. The action and Dockerfiles are tightly coupled to the stacks-core repo, which has a few consequences:

  • There's no benefit to being able to reuse this action in other projects

Correct - the intent here is not to re-use this action, it's meant only to be used by stacks-core workflows.

  • If all stacks-core branches share the same action, this would make refactoring stacks-core difficult, because the Dockerfiles contains build flags and sub-project names that could change

yes, but in this case these Dockerfile are only used for relase builds - PR dockerfile(s) are not being moved.

  • Making a change to how releases are built could easily require 2 PRs, one in stacks-core and one in actions

Possible - but the intent here is that if we notice a change is required to a release build, the fix is much easier/quicker to make vs merging a PR to master in the blockchain repo. it does add a little extra complexity, sure - but the goal is to make it easier to fix issues without having to modify the blockchain repo via PR.

IMO it makes more sense for these files to live in stacks-core

valid points - the problem this aims to solve (which came up testing a new release ci workflow) is allowing for non-code changes outside of the blockchain repo.

what brought about this change is specifically the upcoming addition of the stacks-signer binary - which was not cross-compiling for various reasons, but is enabled by default. the solution was to modify the release Dockerfiles in ./build-scripts to only build specific binaries and not stacks-signer.

this PR further abstracts ci/build related work out of the blockchain repo itself which is objectively a good thing since it allows us to iterate faster on non-code changes.

consider this scenario: if we only used actions/workflows defined in the stacks-core repo, it becomes necessarily more difficult to resolve workflow issues because of the higher bar to merge PR's (i.e. we need to upgrade the git checkout action, for example). PR's like this aim to make those changes easier to accomplish and further DRY any other workflows we may want to use.

wileyj avatar Feb 14 '24 20:02 wileyj

I'm not sure this is an improvement. The action and Dockerfiles are tightly coupled to the stacks-core repo, which has a few consequences:

  • There's no benefit to being able to reuse this action in other projects
  • If all stacks-core branches share the same action, this would make refactoring stacks-core difficult, because the Dockerfiles contains build flags and sub-project names that could change
  • Making a change to how releases are built could easily require 2 PRs, one in stacks-core and one in actions

IMO it makes more sense for these files to live in stacks-core

why i think this PR is needed: https://github.com/stacks-network/stacks-core/pull/4312/files

wileyj avatar Feb 14 '24 20:02 wileyj