fix: atomic commits for multi-stack dependency builds#608
Open
ivanovac wants to merge 10 commits into
Open
Conversation
This commit fixes the race condition where multi-stack dependency builds
(e.g., cflinuxfs4 + cflinuxfs5) would create separate git commits per stack,
causing the update job to only see one stack's metadata file and generate
incomplete buildpack PRs.
Changes:
- Add SKIP_INDIVIDUAL_COMMIT flag to build.sh to skip per-stack commits
- Create merge-build-metadata task to atomically commit all stacks together
- Restructure dependency-builds pipeline to:
1. Build all stacks in parallel with SKIP_INDIVIDUAL_COMMIT=true
2. Upload artifacts to S3 in parallel
3. Merge metadata and create single atomic commit with format:
'Build <dep> - <version> - <stack1>,<stack2>'
- Preserve existing behavior for any-stack and single-stack builds
- Keep SKIP_COMMIT flag intact for parity tests
Root cause: Each parallel build task independently committed to the builds
git repo, creating separate commits. The update job would fetch at a single
commit SHA and only see one stack's JSON file.
Solution: Skip individual commits during parallel builds, then merge all
metadata files and commit atomically in a single step after builds complete.
This ensures buildpack PRs include all built stacks in the manifest.
PR #97 (Go rewrite) was merged to main branch in binary-builder repo. The go-binary-builder-cflinuxfs4-parity branch no longer exists.
The merge.sh script was not properly staging changes after rsync, causing 'unstaged changes' errors when the git resource tried to rebase and push. Now the script unconditionally stages all changes with 'git add' before checking if there's anything to commit.
…uilds When building for multiple stacks (cflinuxfs4 + cflinuxfs5), each stack produces a unique binary with its own URL and SHA256. The manifest must contain separate entries for each stack. Previous behavior: The loop called Dependencies.switch() on each iteration, causing the second stack to replace the first stack's entry. New behavior: - For any-stack builds: Create ONE entry with all stacks (same URL/SHA256) - For multi-stack builds: Create SEPARATE entries per stack (different URLs/SHA256s) This ensures buildpack PRs include all built stacks, not just the last one processed.
Openresty uses 4-part versions (e.g., 1.29.2.3), but SemVer only supports 3-part versions. When SemVer parses "1.29.2.3" and "1.29.2.1", both become "1.29.2" (ignoring the 4th part), causing them to be equal. This prevented the first stack (cflinuxfs4) from being added because latest?() incorrectly returned false. Solution: Fallback to Gem::Version for comparison when: - SemVer parsing fails, OR - SemVer makes different versions appear equal Gem::Version handles arbitrary version part counts correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes the issue where multi-stack dependency builds (e.g., cflinuxfs4 + cflinuxfs5) would only create buildpack PRs with ONE stack instead of both. The root cause was actually two separate bugs that needed fixing.
Problem
When a new dependency version is built for multiple stacks, the resulting buildpack PR should include entries for ALL stacks, but it was only including one.
Evidence: Openresty 1.29.2.3 was built for both cflinuxfs4 and cflinuxfs5, but the update job created nginx-buildpack PR #403 with only cflinuxfs5.
Root Causes
Bug #1: Non-Atomic Commits (Fixed by merge-build-metadata task)
Original behavior: Each parallel build task independently committed to the builds git repo:
Git history example:
Fix: Added merge-build-metadata task that creates a single atomic commit after all stacks complete:
Bug #2: 4-Part Version Comparison Failure (Fixed in dependencies.rb)
The real bug discovered during testing: Even with atomic commits working correctly, the first stack (cflinuxfs4) still wasn't being added to the manifest!
Root cause: Openresty uses 4-part versions (e.g.,
1.29.2.3,1.29.2.1), but the SemVer library only supports 3-part versions (major.minor.patch). Whenlatest?()compared versions:This caused the script to think
1.29.2.3was NOT newer than1.29.2.1, so it refused to add the cflinuxfs4 entry.Debug output that revealed the bug:
Fix: Added fallback to
Gem::Versionwhich correctly handles arbitrary version part counts:Solution
1. Add SKIP_INDIVIDUAL_COMMIT flag to build-binary task
true, build tasks skip individual git commitsSKIP_COMMITflag for parity tests2. Create merge-build-metadata task
Build <dep> - <version> - <stack1>,<stack2>3. Fix version comparison in dependencies.rb
Gem::Versionfallback inlatest?()method4. Restructure dependency-builds pipeline
Multi-stack build jobs now follow this pattern:
5. Update binary-builder resource
go-binary-builder-cflinuxfs4-paritytomainbranchTesting Results
✅ Build Job (build-openresty-1.29.x #16)
fa3a82154a Build openresty - 1.29.2.3 - cflinuxfs4,cflinuxfs5binary-builds-new/openresty/1.29.2.3-cflinuxfs4.jsonbinary-builds-new/openresty/1.29.2.3-cflinuxfs5.json✅ Update Job (update-openresty-1.29.x-nginx #17 - after fixes)
SUCCESS! ✅ Both stacks are now included with their respective URLs and SHA256 hashes.
Files Changed
New files:
tasks/merge-build-metadata/task.yml- Concourse task definitiontasks/merge-build-metadata/merge.sh- Bash script that merges JSON files and commits atomicallyModified files:
tasks/build-binary/build.sh- Add SKIP_INDIVIDUAL_COMMIT flag (lines 151-156)tasks/update-buildpack-dependency/run.rb- Restructured to collect all stacks before processingtasks/update-buildpack-dependency/dependencies.rb- Fixlatest?()to handle 4-part versionspipelines/dependency-builds/pipeline.yml- Restructure multi-stack jobs + update binary-builder branchScope
Applies to: All multi-stack dependency builds in the pipeline (currently only nginx/openresty uses multi-stack builds)
Does NOT affect:
Post-Merge Tasks
After merging this PR:
pipelines/dependency-builds/pipeline.ymlto pointbuildpacks-ciresource back tobranch: master(currently temporarily pointing to PR branch for testing)Related Issues
This resolves the ongoing issue where buildpack PRs for multi-stack dependencies only included one stack, requiring manual intervention to add the missing stack entries.