Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Re-allow building without seccomp installed #8686

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Re-allow building without seccomp installed #8686

merged 2 commits into from
Oct 17, 2024

Conversation

michalsieron
Copy link
Contributor

@michalsieron michalsieron commented Oct 16, 2024 •
edited by saschagrunert
Loading

What type of PR is this?

/kind other

What this PR does / why we need it:

I think #3300 regressed #218 by unconditionally compiling with seccomp enabled. #6488 fixed this for non-linux and non-cgo build platforms, but the issue still exists when one uses a cgo-enabled Linux build host but still wants to build without seccomp. In such case build ends quickly with pkg-config error: Package 'libseccomp' not found.

This patch makes internal/config/seccomp respect the seccomp buildtag.

Which issue(s) this PR fixes:

Fixes #218 regression

Special notes for your reviewer:

Fixed building CRI-O without libseccomp.

Does this PR introduce a user-facing change?

None?

@michalsieron michalsieron requested a review from mrunalp as a code owner October 16, 2024 16:29
@openshift-ci openshift-ci bot added kind/other Categorizes issue or PR as not clearly related to any existing kind/* category dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 16, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 16, 2024

Hi @michalsieron. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 16, 2024
@kwilczynski
Copy link
Member

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 16, 2024
@michalsieron
Re-allow building without seccomp installed
7849e3e 
I think cri-o#3300 regressed cri-o#218 by unconditionally compiling with seccomp
enabled. cri-o#6488 fixed this for non-linux and non-cgo build platforms, but
the issue still exists when one uses a cgo-enabled Linux build host
but still wants to build without seccomp. In such case build ends quickly
with pkg-config error: `Package 'libseccomp' not found`.

This patch makes internal/config/seccomp respect the seccomp buildtag.

Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
@michalsieron
seccomp_unsupported.go: Fix lint issues
a271b4a 
Added missing dots at the end of comments and ran through gofumpt.

Signed-off-by: Michal Sieron <michalwsieron@gmail.com>
@michalsieron michalsieron force-pushed the reallow-build-without-seccomp branch from 8719b90 to a271b4a Compare October 16, 2024 19:10
@michalsieron
Copy link
Contributor Author

@kwilczynski I think I fixed lint issues, but I don't really understand the test failures 😕

@saschagrunert
Copy link
Member

/ok-to-test
/override ci/prow/ci-e2e-evented-pleg

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 17, 2024

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/ci-e2e-evented-pleg

In response to this:

/ok-to-test
/override ci/prow/ci-e2e-evented-pleg

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 kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 17, 2024
@kwilczynski
Copy link
Member

/approve
/lgtm

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwilczynski, michalsieron, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov Codecov
Copy link

codecov bot commented Oct 17, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.80%. Comparing base (7049bb9) to head (a271b4a).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8686      +/-   ##
==========================================
- Coverage   48.82%   48.80%   -0.02%     
==========================================
  Files         153      153              
  Lines       17388    17388              
==========================================
- Hits         8489     8487       -2     
- Misses       7836     7837       +1     
- Partials     1063     1064       +1

@openshift-merge-bot openshift-merge-bot bot merged commit 24d68f5 into cri-o:main Oct 17, 2024
81 of 82 checks passed
@michalsieron michalsieron deleted the reallow-build-without-seccomp branch October 17, 2024 09:51
@michalsieron michalsieron mentioned this pull request Dec 6, 2024
Merged
@github-actions github-actions bot mentioned this pull request Dec 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@hasan4791 hasan4791 Awaiting requested review from hasan4791

@sohankunkerkar sohankunkerkar Awaiting requested review from sohankunkerkar

Assignees

@kwilczynski kwilczynski

@saschagrunert saschagrunert

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/other Categorizes issue or PR as not clearly related to any existing kind/* category lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

libseccomp is required for compilation
3 participants
@michalsieron @kwilczynski @saschagrunert