fix: avoid duplicate --platform argument in docker-buildx#141
fix: avoid duplicate --platform argument in docker-buildx#141RongGu merged 1 commit intosgl-project:mainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the docker-buildx Makefile target was adding a duplicate --platform argument when the Dockerfile already contained it, causing build failures.
Changes:
- Added a conditional check in the
docker-buildxtarget to detect if--platformis already present in the Dockerfile before attempting to add it - Updated the comment to reflect the new conditional behavior
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross, and preserve the original Dockerfile | ||
| sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross | ||
| # copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross if not already present | ||
| if grep -q "^FROM.*--platform" Dockerfile; then \ |
There was a problem hiding this comment.
The grep pattern "^FROM.*--platform" could produce false positives if --platform appears elsewhere on a FROM line (e.g., in a comment like "FROM image # --platform note"). While unlikely in practice, consider using a more precise pattern like "^FROM[[:space:]]+--platform" to ensure --platform is an actual argument to FROM and appears immediately after FROM with whitespace separation.
| if grep -q "^FROM.*--platform" Dockerfile; then \ | |
| if grep -q "^FROM[[:space:]]\+--platform" Dockerfile; then \ |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where a duplicate --platform argument could be added during the docker-buildx process. The approach of checking for an existing platform argument before adding it is sound. I've provided one suggestion to further improve the implementation by replacing the if/grep/sed logic with a more concise and robust awk command. This change simplifies the Makefile and handles potential edge cases more gracefully.
|
@onenewcode Please take a look at the comments. Thanks. |
Fix the issue where docker-buildx target adds duplicate --platform argument when Dockerfile already contains it. Add a check to detect if Dockerfile already has --platform parameter before inserting it. This resolves the build error: "duplicate flag specified: --platform" that occurs when running make docker-buildx. Fixes sgl-project#136 Signed-off-by: onenewcode <[email protected]>
I have made modifications based on the AI's suggestions. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ⅰ. Motivation
Fix the issue where docker-buildx target adds duplicate --platform argument when Dockerfile already contains it. Add a check to detect if Dockerfile already has --platform parameter before inserting it.
This resolves the build error: "duplicate flag specified: --platform" that occurs when running make docker-buildx.
Ⅱ. Modifications
Modify Makefile: Check if the Dockerfile already contains the --platform parameter before insertion to avoid duplicate additions.
Ⅲ. Does this pull request fix one issue?
fixes #136
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
No unit tests are needed for this change.
Ⅴ. Describe how to verify it
run
make docker-buildxVI. Special notes for reviews
Checklist
make fmt.