NO-JIRA: Fix oc builds on Mac OS X Apple Silicon machines - #2246
Conversation
|
@tchap: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe ChangesGSSAPI dependency update
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: 🔵 Low · up to The change dynamically selects the Homebrew include path for Apple Silicon macOS builds and updates the vendored dependency metadata. The PR is mergeable with owner awareness that the vendor tree should be regenerated or verified and the Darwin/arm64 build confirmed, since inconsistency could cause affected builds to fail. Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
f6ccb70 to
b34912e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 47-50: The Makefile currently overwrites CGO_CFLAGS when setting
the Heimdal include path; update the assignment so it appends the brew-provided
include directory instead of replacing existing flags. Locate the
export/assignment that references CGO_CFLAGS and the Heimdal path (the line
using brew --prefix and opt/heimdal/include) and change it to append the
-I$(shell brew --prefix)/opt/heimdal/include to the existing CGO_CFLAGS value
(preserving any caller/CI-provided flags) and keep the export so CGO_CFLAGS
remains exported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2fadb9a2-bc1b-4bde-a702-1726062f5781
📒 Files selected for processing (1)
Makefile
bf34b8f to
6cde768
Compare
|
/retest |
| ifneq (,$(shell command -v brew 2>/dev/null)) | ||
| HEIMDAL_PREFIX := $(shell brew --prefix heimdal 2>/dev/null) | ||
| ifneq (,$(HEIMDAL_PREFIX)) | ||
| export CGO_CFLAGS := $(CGO_CFLAGS) -I$(HEIMDAL_PREFIX)/include |
There was a problem hiding this comment.
I know that I resisted hard to not touch https://github.com/openshift/gssapi repository and forced you updating here.
But these changes look like either we need to simply update https://github.com/openshift/gssapi/blob/5fb4217df13b8e6878046fe1e5c10e560e1b86dc/name.go#L21 as you mentioned before or leave this issue opened rather than adding a hacky change in oc.
|
/hold until openshift/gssapi#4 is merged. I will update the dep after. |
|
We can't merge openshift/gssapi#4 without verifying it works. I'd recommend replace with your fork&branch and get the signal from CI (to ensure that there is no regression). I'd also run all payload blocking jobs.Lastly, we should find a way to test this on darwin/arm64. Once everything is great, we can start seeking staff engineer to merge openshift/gssapi#4 :) |
|
Sure, I am gonna update this PR now to match the PR in |
6cde768 to
bcc1b73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 223-224: The go.mod replace directive currently pointing at the
personal fork github.com/tchap/gssapi (replace github.com/apcera/gssapi =>
github.com/tchap/gssapi ...) introduces supply-chain risk; update the replace to
point to the official openshift fork (github.com/openshift/gssapi) once
openshift/gssapi#4 is merged, or add a short TODO and create/link an issue to
track migration, and ensure the replace line is removed or updated to the
released tag/commit of github.com/openshift/gssapi in go.mod (reference the
replace directive and the module name github.com/apcera/gssapi to find the
change).
- Around line 223-224: The go.mod replace directive referencing
"github.com/tchap/gssapi" is pointing to a non-public fork and will break public
builds; either confirm the fork is intentionally private and document required
access/authentication, or replace/remove the replace directive so it references
a publicly accessible module. Specifically, update the replace line in go.mod
(the "replace github.com/apcera/gssapi => github.com/tchap/gssapi
v0.0.0-...-047531cb33ea" entry) to the correct public fork/path or revert to the
upstream "github.com/apcera/gssapi", then run "go get" / "go mod tidy" to
refresh go.sum and verify builds succeed; if the private fork is required, add
documentation in the repo explaining how to authenticate and fetch that
dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d95104ee-2805-4ed3-83f2-9e1abfdcb325
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumvendor/github.com/apcera/gssapi/name.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (2)
Makefilego.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
bcc1b73 to
19fda0a
Compare
19fda0a to
11e9104
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
I vendored the updated /unhold |
|
I think, it is not possible to create regression with that change. |
|
Scheduling required tests: Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, tchap The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@tchap: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/verified by CI |
|
@tchap: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Homebrew uses a different prefix path than is hard-coded in the
gssapilibrary.This change sets the include path dynamically based on Homebrew, when available.
Fixes #2136
I also cleaned up the Makefile a bit in a separate commit.
Summary by CodeRabbit