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

add(crit): other os support #197

Merged
merged 2 commits into from
Jan 14, 2025
Merged

Conversation

cg10036
Copy link
Contributor

@cg10036 cg10036 commented Jan 5, 2025

When using CRIT on macOS, the following errors occur:

# github.com/checkpoint-restore/go-criu/v7/crit
../../../go/pkg/mod/github.com/checkpoint-restore/go-criu/[email protected]/crit/utils.go:329:15: undefined: syscall.AF_NETLINK
../../../go/pkg/mod/github.com/checkpoint-restore/go-criu/[email protected]/crit/utils.go:331:15: undefined: syscall.AF_BRIDGE
../../../go/pkg/mod/github.com/checkpoint-restore/go-criu/[email protected]/crit/utils.go:333:15: undefined: syscall.AF_KEY
../../../go/pkg/mod/github.com/checkpoint-restore/go-criu/[email protected]/crit/utils.go:335:15: undefined: syscall.AF_PACKET
../../../go/pkg/mod/github.com/checkpoint-restore/go-criu/[email protected]/crit/utils.go:359:15: undefined: syscall.SOCK_PACKET
../../../go/pkg/mod/github.com/checkpoint-restore/go-criu/[email protected]/crit/utils.go:381:15: undefined: syscall.IPPROTO_UDPLITE

I believe that CRIT can be sufficiently supported even on non-Linux platforms. Therefore, I have added the necessary support.

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 49.80%. Comparing base (47757dc) to head (b9d8fad).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
crit/utils.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   48.83%   49.80%   +0.97%     
==========================================
  Files          21       21              
  Lines        2353     2307      -46     
==========================================
  Hits         1149     1149              
+ Misses       1065     1019      -46     
  Partials      139      139              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adrianreber
Copy link
Member

Thanks. Can you add a CI run on a non Linux platform. To ensure we do not break non Linux support in the future.

@cg10036
Copy link
Contributor Author

cg10036 commented Jan 5, 2025

I have added tests for macOS. However, since I only have access to a Mac, I was unable to create tests for the Windows environment. Additionally, I haven't written many tests before, so I'm not sure if I did them correctly. Could you please review the changes? Thank you.

.github/workflows/main.yml Outdated Show resolved Hide resolved
crit/mempages_other.go Outdated Show resolved Hide resolved
crit/utils_linux.go Outdated Show resolved Hide resolved
@adrianreber
Copy link
Member

@rst0git @snprajwal The failures using the criu master branch is probably related to outdated image protobuf definitions. Can anyone of you update the definitions in this repository to the latest version from CRIU upstream.

@snprajwal
Copy link
Member

@cg10036 could you please rebase your changes on top of the master branch? Also, it would be great if you could squash the commits down into a single logical commit :) we tend to avoid fixup commits in general, since they pollute the commit history of the project.

@cg10036
Copy link
Contributor Author

cg10036 commented Jan 7, 2025

I've rebased and squashed the commits as requested. Let me know if you need anything else!

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
@adrianreber
Copy link
Member

@rst0git @snprajwal Looks ready to me. Anything from your side?

Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

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

LGTM

@cg10036 Thank you for working on this!

Copy link
Member

@snprajwal snprajwal left a comment

Choose a reason for hiding this comment

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

LGTM!

@adrianreber adrianreber merged commit 9996dd5 into checkpoint-restore:master Jan 14, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants