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.

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

Update go-capnp package to fix build issue on Go 1.23 #2317

Conversation

kwilczynski
Copy link
Member

@kwilczynski kwilczynski commented Jul 21, 2024

What type of PR is this?

/kind dependency-change

What this PR does / why we need it:

When building the go-capnp using Go 1.23 or Go tip, the following build error will be reported (an example build failure from the CRI-O project):

# capnproto.org/go/capnp/v3
vendor/capnproto.org/go/capnp/v3/list.go:1087:10: invalid composite literal type T

Thus, upgrade the go-capnp version to v3.0.1-alpha.2, which would fix the build issue on a newer version of Go.

Related:

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

None

Copy link
Contributor

openshift-ci bot commented Jul 21, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kwilczynski
Once this PR has been reviewed and has the lgtm label, please assign rphillips for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@kwilczynski
Copy link
Member Author

/assign kwilczynski

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.34%. Comparing base (4e0f474) to head (d9afa0d).
Report is 603 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2317      +/-   ##
==========================================
- Coverage   37.53%   37.34%   -0.20%     
==========================================
  Files          15       15              
  Lines        1268     1264       -4     
  Branches      414      421       +7     
==========================================
- Hits          476      472       -4     
+ Misses        526      524       -2     
- Partials      266      268       +2     

@saschagrunert
Copy link
Member

saschagrunert commented Jul 22, 2024

Integration tests seems to fail, do you know why?

@kwilczynski
Copy link
Member Author

/close

@openshift-ci openshift-ci bot closed this Jul 22, 2024
Copy link
Contributor

openshift-ci bot commented Jul 22, 2024

@kwilczynski: Closed this PR.

In response to this:

/close

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.

@kwilczynski
Copy link
Member Author

/reopen

@openshift-ci openshift-ci bot reopened this Jul 22, 2024
Copy link
Contributor

openshift-ci bot commented Jul 22, 2024

@kwilczynski: Reopened this PR.

In response to this:

/reopen

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.

@kwilczynski
Copy link
Member Author

Integration tests seems to fail, do you know why?

@saschagrunert, I will look into it.

@kwilczynski
Copy link
Member Author

Integration tests seems to fail, do you know why?

@saschagrunert, I will look into it.

@saschagrunert, I was able to narrow the failures down to the following change of the go-capnp package:

Part of the following release:

Neet to look whether we need to do something or whether this is a regression.

@kwilczynski kwilczynski force-pushed the feature/update-go-capnp-package branch from a59b5e4 to d5786ec Compare July 30, 2024 14:15
@kwilczynski kwilczynski force-pushed the feature/update-go-capnp-package branch 3 times, most recently from c13d651 to ec23245 Compare July 30, 2024 15:19
@kwilczynski kwilczynski changed the title Update go-capnp package to fix build issue on Go 1.23 [WIP] Update go-capnp package to fix build issue on Go 1.23 Jul 30, 2024
Signed-off-by: Krzysztof Wilczyński <kwilczynski@redhat.com>
@kwilczynski kwilczynski force-pushed the feature/update-go-capnp-package branch 3 times, most recently from 947f470 to bdd2bb7 Compare July 31, 2024 08:21
@saschagrunert
Copy link
Member

Yay, green CI! 🟢

Signed-off-by: Krzysztof Wilczyński <kwilczynski@redhat.com>
@saschagrunert
Copy link
Member

@kwilczynski is this still WIP?

Signed-off-by: Krzysztof Wilczyński <kwilczynski@redhat.com>
Signed-off-by: Krzysztof Wilczyński <kwilczynski@redhat.com>
@kwilczynski kwilczynski force-pushed the feature/update-go-capnp-package branch from bdd2bb7 to d9afa0d Compare July 31, 2024 10:29
@kwilczynski kwilczynski changed the title [WIP] Update go-capnp package to fix build issue on Go 1.23 Update go-capnp package to fix build issue on Go 1.23 Jul 31, 2024
@kwilczynski
Copy link
Member Author

@kwilczynski is this still WIP?

@saschagrunert, should be ready for review, etc.

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@saschagrunert
Copy link
Member

/approve

@openshift-merge-bot openshift-merge-bot bot merged commit b0b1518 into containers:main Jul 31, 2024
33 checks passed
@kwilczynski kwilczynski deleted the feature/update-go-capnp-package branch July 31, 2024 11:07
@kwilczynski
Copy link
Member Author

kwilczynski commented Aug 1, 2024

@saschagrunert, do you think we could cut a new release?

@saschagrunert
Copy link
Member

@saschagrunert, do you think we could cut a new release?

Yes, let me first check if we can / have to fix cri-o/cri-o#8411. I'm not even sure if that's a conmon-rs issue.

@kwilczynski
Copy link
Member Author

@saschagrunert, do you think we could cut a new release?

Yes, let me first check if we can / have to fix cri-o/cri-o#8411. I'm not even sure if that's a conmon-rs issue.

@saschagrunert, the exit code and time out boolean value should be copied so the correct exit code should be reflected. We have a check in a test to verify this, per:

I also doubt that our change here resolved anything for the other issue.

@saschagrunert
Copy link
Member

@kwilczynski see https://github.com/kubernetes-sigs/cri-tools/actions/runs/10195323313/job/28204264894?pr=1546

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants