-
Notifications
You must be signed in to change notification settings - Fork 16
build: set macOS 14 minimum version #155
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe Makefile’s darwin-arm64 build target now enables CGO and sets macOS deployment flags to enforce a minimum version of 14.0 when building the plugin. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @doringeman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the build configuration for macOS ARM64 targets by enforcing a minimum operating system version of macOS 14.0. This change ensures that the compiled binaries are correctly tagged for compatibility with macOS Sonoma, which can be crucial for leveraging new platform features or adhering to specific deployment requirements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
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.
Code Review
This pull request updates the Makefile to set the minimum macOS version to 14.0 for the darwin/arm64 release build. The change correctly uses CGO flags to set the deployment target. I have one suggestion to improve maintainability by defining the version number in a variable to avoid hardcoding values.
Makefile
Outdated
| fi | ||
| @echo "Building release version '$(VERSION)'..." | ||
| GOOS=darwin GOARCH=arm64 go build -trimpath -ldflags="-s -w -X github.com/docker/model-cli/desktop.Version=$(VERSION)" -o dist/darwin-arm64/$(PLUGIN_NAME) . | ||
| GOOS=darwin GOARCH=arm64 CGO_ENABLED=1 CGO_CFLAGS="-mmacosx-version-min=14.0" CGO_LDFLAGS="-mmacosx-version-min=14.0" go build -trimpath -ldflags="-s -w -X github.com/docker/model-cli/desktop.Version=$(VERSION)" -o dist/darwin-arm64/$(PLUGIN_NAME) . |
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.
To improve maintainability and avoid hardcoding values, consider defining the minimum macOS version as a variable at the top of the Makefile. This makes it easier to find and update in the future.
Additionally, since CGO_CFLAGS and CGO_LDFLAGS have the same value, you could define another variable to reduce duplication.
For example:
# At the top of the Makefile
MACOS_MIN_VERSION := 14.0
MACOS_MIN_VERSION_LDFLAG := -mmacosx-version-min=$(MACOS_MIN_VERSION)
# ... inside the release target
GOOS=darwin GOARCH=arm64 CGO_ENABLED=1 CGO_CFLAGS="$(MACOS_MIN_VERSION_LDFLAG)" CGO_LDFLAGS="$(MACOS_MIN_VERSION_LDFLAG)" go build ...|
Right, we can't do this because we cross-compile on Linux. |
ericcurtin
left a comment
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.
Good idea, the older we can go back the better
Did you encounter a machine we were breaking on? |
|
https://github.com/docker/model-cli/actions/runs/17911633186/job/50924317009?pr=155. |
Sorry, I meant was there a macOS machine these binaries were deployed to that failed? |
|
No, we actually built it on macOS 15 and it worked on 14 as well. See: https://github.com/docker/model-cli-release/pull/5#issuecomment-3312968950. I'm also OK to drop CGO and rely on Go's runtime for macOS 14 as well, especially that model-cli doesn't interact with the system API in a fancy way. |
| jobs: | ||
| build: | ||
| runs-on: ubuntu-latest | ||
| runs-on: macos-latest |
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.
This seems like a good idea, why cross-compile when we can do normal compiles :)
This could make it easier to run a few smoke tests also, post-build.
xenoscopic
left a comment
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.
LGTM; only thing I could suggest would be doing the variable extraction suggested by Gemini.
c88c9cf to
8523c51
Compare
Signed-off-by: Dorin Geman <[email protected]>
Signed-off-by: Dorin Geman <[email protected]>
8523c51 to
5f05571
Compare
Summary by Sourcery
Build: