Add CPU_INFO_FLAGS for Apple Silicon #2694
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant OS
User->>Makefile: Trigger build process
Makefile->>OS: Check operating system
OS-->>Makefile: Return OS type (Linux or macOS)
alt macOS
Makefile->>OS: Retrieve CPU brand string using sysctl
OS-->>Makefile: Return CPU brand string
else Linux
Makefile->>Makefile: Set CPU_INFO_FLAGS to empty
end
Makefile->>OS: Evaluate number of processing cores
OS-->>Makefile: Return number of cores
Makefile->>User: Complete build process
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Makefile (1)
130-131: LGTM! Consider minor improvement for consistency.The changes to
CPU_INFO_FLAGSfor Darwin systems look good. It now properly handles Apple Silicon by attempting to get the CPU brand string and falling back to "Apple Silicon" if that fails.For consistency, you might want to consider using the same error handling approach for the
CORESassignment:-CORES := $(eval CORES := $(shell sysctl -n hw.ncpu 2>/dev/null || getconf _NPROCESSORS_ONLN 2>/dev/null))$(CORES) +CORES := $(eval CORES := $(shell sysctl -n hw.ncpu 2>/dev/null || getconf _NPROCESSORS_ONLN 2>/dev/null || echo "1"))$(CORES)This ensures that
CORESalways has a value, even if bothsysctlandgetconffail.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2694 +/- ##
=======================================
Coverage 24.04% 24.04%
=======================================
Files 539 539
Lines 46792 46792
=======================================
+ Hits 11250 11253 +3
+ Misses 34766 34762 -4
- Partials 776 777 +1 ☔ View full report in Codecov by Sentry. |
kpango
requested review from
a team,
vankichi and
datelier
and removed request for
a team
Signed-off-by: kpango <[email protected]>
kpango
force-pushed
the
refactor/makefile/build-cpu-info-for-apple-silicon
branch
from
4c76685 to
d64e797
Compare
Deploying vald with
|
| Latest commit: |
467d469
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7c3952de.vald.pages.dev |
| Branch Preview URL: | https://refactor-makefile-build-cpu.vald.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
dockers/agent/core/agent/Dockerfile (1)
Line range hint
1-1: Consider removing unusedUPX_OPTIONSargumentThe Dockerfile defines an
UPX_OPTIONSargument in the first line:ARG UPX_OPTIONS=-9However, this argument is not used anywhere else in the Dockerfile. If it's not needed for other parts of the build process or in scripts that use this Dockerfile, consider removing it to keep the Dockerfile clean and avoid confusion.
If it is used elsewhere, it might be helpful to add a comment explaining its purpose and where it's used.
dockers/ci/base/Dockerfile (1)
Line range hint
1-124: Consider security enhancements and image size optimizationThe Dockerfile follows many best practices, such as using multi-stage builds, caching mechanisms, and grouping installation commands. However, consider the following suggestions for further improvement:
Security: Add a non-root user for running the container. While the current setup uses root, it's generally recommended to use a non-root user for security reasons.
Image size: Consider using a smaller base image if possible, such as
debian:slimoralpine, to reduce the overall image size.Clean-up: Although you're using multi-stage builds, ensure all unnecessary build dependencies are removed in the final stage to minimize the image size.
Version pinning: For better reproducibility, consider pinning specific versions of installed packages where possible.
HEALTHCHECK: Consider adding a HEALTHCHECK instruction to allow Docker to test if your container is still working.
Here's an example of how you might implement some of these suggestions:
# Use a non-root user RUN useradd -m appuser USER appuser # Add a HEALTHCHECK HEALTHCHECK CMD curl --fail http://localhost:8080/health || exit 1 # Example of version pinning (adjust versions as needed) RUN apt-get install -y --no-install-recommends \ curl=7.68.0-1ubuntu2.7 \ git=1:2.25.1-1ubuntu3.5 \ # ... other packagesThese changes can enhance the security and efficiency of your Docker image while maintaining its functionality.
dockers/dev/Dockerfile (1)
Line range hint
1-48: Overall impact is positive, consider adding a comment for clarityThe addition of the
RUSTUP_HOMEenvironment variable is a good improvement to the Dockerfile. It aligns well with the existing Rust-related configurations and doesn't introduce any conflicts or issues with other parts of the file.While this change is consistent with best practices for Rust development environments, it's not directly related to the PR objective of adding CPU_INFO_FLAGS for Apple Silicon. However, it's a valuable addition that enhances the overall quality of the development environment.
Consider adding a comment above the Rust-related environment variables to explain their purpose and relationship. This could improve readability for future maintainers. For example:
+# Rust-related environment variables ENV RUST_HOME=/usr/local/lib/rust ENV CARGO_HOME=${RUST_HOME}/cargo ENV RUSTUP_HOME=${RUST_HOME}/rustup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumrust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
- Makefile (1 hunks)
- dockers/agent/core/agent/Dockerfile (1 hunks)
- dockers/ci/base/Dockerfile (1 hunks)
- dockers/dev/Dockerfile (1 hunks)
- go.mod (2 hunks)
- versions/TELEPRESENCE_VERSION (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- versions/TELEPRESENCE_VERSION
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🔇 Additional comments (5)
dockers/agent/core/agent/Dockerfile (2)
43-43: LGTM: Proper setup of Rust environment variablesThe addition of
ENV CARGO_HOME=${RUST_HOME}/cargois correct and aligns with best practices for Rust development environments. This change ensures that Cargo (Rust's package manager) has a specific home directory, which is essential for proper functioning of Rust tools and dependencies.The
CARGO_HOMEis correctly set based on the previously definedRUST_HOME, and thePATHis appropriately updated to include the Cargo binary directory. This setup will allow for consistent and predictable behavior of Rust tools within the Docker container.
Line range hint
1-93: Overall, the Dockerfile looks well-structured and follows good practicesThe Dockerfile uses multi-stage builds, caching mechanisms, and a minimal distroless image for the final stage, which are all excellent practices for creating efficient and secure Docker images. The addition of the
CARGO_HOMEenvironment variable improves the Rust development environment setup.The only minor suggestion is to consider removing the unused
UPX_OPTIONSargument if it's not needed elsewhere in the build process.Great job on maintaining a clean and efficient Dockerfile!
dockers/ci/base/Dockerfile (1)
48-48: LGTM: Addition of CARGO_HOME environment variableThe addition of the
CARGO_HOMEenvironment variable is a good practice for Rust development environments. It ensures that Cargo's files are stored in a consistent location, which is particularly important in containerized environments. This change aligns well with Rust's recommended setup and complements the existingRUSTUP_HOMEvariable.dockers/dev/Dockerfile (1)
48-48: LGTM: Explicit definition of RUSTUP_HOME enhances clarityThe addition of
ENV RUSTUP_HOME=${RUST_HOME}/rustupis a positive change. It explicitly defines the location for Rust toolchain installations, which enhances clarity and follows best practices for Rust development environments. This change is consistent with the existing structure and complements the previously definedCARGO_HOME.Moreover, it ensures that the
${RUSTUP_HOME}/bindirectory included in thePATH(defined in the next line) points to a specific, well-defined location. This can help prevent potential issues related to Rust toolchain discovery and usage within the container.go.mod (1)
296-296: LGTM: Dependency version updatesThe updates to
gocloud.dev(v0.39.0 to v0.40.0) andsigs.k8s.io/json(v0.0.0-20241009153224-e386a8af8d30 to v0.0.0-20241010143419-9aa6b5e7a4b3) are minor version changes, which typically indicate bug fixes or small improvements. These updates are appropriate and align with good dependency management practices.Also applies to: 345-345
datelier
deleted the
refactor/makefile/build-cpu-info-for-apple-silicon
branch
Signed-off-by: kpango <[email protected]>
Signed-off-by: kpango <[email protected]> Co-authored-by: Yusuke Kato <[email protected]>
Signed-off-by: kpango <[email protected]>
Signed-off-by: kpango <[email protected]>
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit