-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: no amd64 binaries on manual installation #37841
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
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThe PR removes problematic sharp dependency re-installation from the Alpine Dockerfile and extends platform-specific binary cleanup during Docker builds. Additionally, yarn configuration is updated with a new cache key version and architecture-aware settings to improve multi-platform dependency resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
853c422 to
375479b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37841 +/- ##
===========================================
+ Coverage 67.62% 67.72% +0.10%
===========================================
Files 3463 3463
Lines 113694 113712 +18
Branches 20902 20910 +8
===========================================
+ Hits 76881 77012 +131
+ Misses 34681 34530 -151
- Partials 2132 2170 +38
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
447550d to
279a7a4
Compare
63bec15 to
42b3412
Compare
42b3412 to
76f1db9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/actions/setup-node/action.yml (1)
54-55: Cache key update looks appropriate.Adding
.yarnrc.ymlhash and bumping the version suffix to v5 ensures the cache is invalidated when yarn configuration changes, which aligns with the newsupportedArchitecturesconfig being added.The version jump from v3 to v5 is notable—please verify this doesn't conflict with any in-flight branches that might be using v4. If coordinating cache invalidation across multiple branches, consider documenting the version progression.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/actions/build-docker/action.yml(1 hunks).github/actions/setup-node/action.yml(2 hunks)apps/meteor/.docker/Dockerfile.alpine(1 hunks)
🔇 Additional comments (5)
.github/actions/build-docker/action.yml (3)
78-78: LGTM on comment update.The updated comment accurately reflects the expanded cleanup operations.
89-93: [Rewritten review comment]
[Exactly ONE classification tag]
87-87: I was unable to complete verification of this review comment due to inability to access the repository. While I confirmed through external sources that linuxmusl-x64 binaries cannot be used on the linux-x64 platform, I could not:
- Examine the actual file and surrounding context to verify the claims about line 87
- Locate issue #37705 to confirm the reported problem
- Determine the intended output of this build action (Docker image, distribution tarball, etc.)
The critical distinction needed: since this file is named
build-docker/action.yml, it likely creates Docker images. Alpine Linux requires rebuilding sharp with --libc=musl, which would make keeping only musl binaries correct for Alpine-based Docker images. However, if this action also produces artifacts for manual installation on glibc systems, that would be a different concern.Without access to the repository context and issue #37705, I cannot definitively determine whether this is a critical issue or appropriate for the action's intended purpose.
.github/actions/setup-node/action.yml (1)
78-82: The yarn config command does not fetch multi-platform binaries.The
supportedArchitecturessetting only controls which packages are acceptable for the current platform, not cross-platform fetching. Yarn still installs binaries only for the current platform duringyarn install. This means lines 78-82 don't actually fetch both glibc and musl binaries as stated in the original comment.The issue with missing glibc binaries for manual installation is not a consequence of fetching and deleting them—those binaries were never fetched in the first place. The build-docker action (line 87) removes non-musl binaries only if they were somehow included in the distribution tarball.
To address the glibc binary issue: either ensure glibc binaries are built and included in the distribution tarball, or document that manual installations require Alpine Linux compatibility.
apps/meteor/.docker/Dockerfile.alpine (1)
11-12: The Alpine Dockerfile is a runtime environment, not the source of distribution tarballs—this concern is incorrect.The Dockerfile.alpine downloads and extends a pre-built tarball rather than creating one. The sharp incompatibility issue specific to Alpine is that when meteor build pre-compiles sharp, it is not compatible with Alpine's libc libraries, which is why the reinstall approach was originally needed. Removing the unnecessary reinstall step is appropriate for Alpine's runtime environment.
This change does not affect the distribution tarballs available for manual Linux installations, so it does not address or interfere with the glibc binary issue described in #37705. The tarball source would need to be verified in the release/CI process, not in the Alpine Dockerfile.
Likely an incorrect or invalid review comment.
|
/patch |
after that just run |
|
/patch |
|
Pull request #37900 added to Project: "Patch 7.13.2" |
Closes #37705
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.