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

crit: fix proto imports for library #109

Merged
merged 4 commits into from
May 20, 2023

Conversation

snprajwal
Copy link
Member

@snprajwal snprajwal commented Mar 26, 2023

When using CRIT as a library, all proto bindings are imported and statically linked. This PR aims to fix this by moving each proto binding into its own package and importing all of them only for the CLI to use. The library will be refactored to allow the user to pass the struct instead of deducing it using handler.go

Tasks:

  • Use Python script to generate bindings
  • Add the imports into handler.go
  • Refactor CRIT to receive the struct as an argument

@adrianreber
Copy link
Member

Couple of ideas which would be nice to have.

Can you introduce a markdown lint runt like https://github.com/checkpoint-restore/checkpointctl/blob/main/.github/workflows/markdown-lint.yml

This would probably also mean to change the existing top-level README.md. Maybe do the introduction of the markdown lint run as a separate PR.

With the introduction of Python code if would be nice to run the Python code also through a linter like flake8. I think the main CRIU repository has some examples how to use flake8.

@snprajwal
Copy link
Member Author

Got it. I've opened a PR for the markdown linter, I'll add a Python linter along with the script in this PR.

@snprajwal snprajwal force-pushed the fix-proto-gen branch 4 times, most recently from d876b9f to efa03cd Compare April 4, 2023 12:32
.github/workflows/main.yml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Patch coverage: 46.27% and project coverage change: -0.51 ⚠️

Comparison is base (45baa8a) 48.84% compared to head (c0cfc38) 48.33%.

❗ Current head c0cfc38 differs from pull request most recent head e69417e. Consider uploading reports for the commit e69417e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
- Coverage   48.84%   48.33%   -0.51%     
==========================================
  Files          22       23       +1     
  Lines        2381     2435      +54     
==========================================
+ Hits         1163     1177      +14     
- Misses       1059     1084      +25     
- Partials      159      174      +15     
Impacted Files Coverage Δ
crit/decode-extra.go 0.00% <0.00%> (ø)
crit/encode-extra.go 0.00% <0.00%> (ø)
crit/encode.go 19.16% <0.00%> (ø)
scripts/magic-gen/magicgen.go 85.00% <ø> (ø)
crit/cli/handler.go 25.00% <25.00%> (ø)
crit/utils.go 36.60% <40.90%> (ø)
test/crit/main.go 61.53% <47.05%> (-4.70%) ⬇️
crit/image.go 49.46% <50.00%> (+1.54%) ⬆️
crit/cli/cli.go 66.49% <58.51%> (ø)
crit/decode.go 27.21% <62.50%> (-1.46%) ⬇️
... and 5 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@snprajwal snprajwal force-pushed the fix-proto-gen branch 2 times, most recently from a2f943a to 10943fa Compare April 26, 2023 18:05
@snprajwal snprajwal marked this pull request as ready for review April 26, 2023 18:08
@snprajwal snprajwal force-pushed the fix-proto-gen branch 2 times, most recently from 0b053c6 to 3697421 Compare April 26, 2023 18:45
@snprajwal
Copy link
Member Author

snprajwal commented Apr 26, 2023

Upon importing the library locally with a simple Go file that looks like below:

package main

import ...

func main() {
    critter := crit.New(...)
    fmt.Println(critter)
}

I noted the following numbers:

  • The number of vendored files in crit/images dropped from 145 to 52
  • The binary size dropped from 7.3M to 7M, shaving off almost 300KB in the process

@adrianreber
Copy link
Member

This PR get really big. Can you move the the last three commits to a separate commit. That looks like something we can merge easily and then you can rebase upon that.

test/Makefile Outdated Show resolved Hide resolved
Since the proto-gen script will also be placed in the scripts directory,
it is ideal to have each script in its own subdirectory along with the
associated tests and Makefile.

Signed-off-by: Prajwal S N <prajwalnadig21@gmail.com>
Previously, CRIT was using an overengineered Makefile stunt to generate
protobuf bindings for the CLI. The problem of statically linking all the
bindings while using the library as a dependency cannot be resolved in a
reasonable manner in the Makefile. The logic has been moved out into a
Python script.

Signed-off-by: Prajwal S N <prajwalnadig21@gmail.com>
Using the new Python script, the protobuf bindings are now generated in
independent packages. This commit refactors CRIT to work with the new
organisation of files. It does not change the implementation of CRIT
in any way apart from resolving the imports correctly.

Signed-off-by: Prajwal S N <prajwalnadig21@gmail.com>
The proto struct required for encoding or decoding the images was
previously being inferred from the `images.ProtoHandler()` function.
Now, it is inferred using that only in the CLI, and is passed to the
actual CRIT functions as a parameter. This eliminates the redundant
protobuf bindings being imported when using CRIT as a library.

Signed-off-by: Prajwal S N <prajwalnadig21@gmail.com>
@snprajwal
Copy link
Member Author

snprajwal commented May 13, 2023

Since we have improved error handling substantially in this PR, the coverage has gone down slightly (we cannot test the failure cases like failing to open file, failing to write to file, etc.)

rst0git

This comment was marked as duplicate.

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

@adrianreber
Copy link
Member

@snprajwal Can you post the binary size of phaul (in test) before and after this PR?

A hello world in go needs 1.8MB on my system and the phaul binary needs 7.7MB. What size do you see of the phaul test binary after your changes?

@snprajwal
Copy link
Member Author

What size do you see of the phaul test binary after your changes?

@adrianreber The phaul binary currently sits at 7.2M on my system

@rst0git
Copy link
Member

rst0git commented May 20, 2023

What size do you see of the phaul test binary after your changes?

@adrianreber The phaul binary currently sits at 7.2M on my system

The following results show the size of the phaul binary on my system (Fedora 37 with go version 1.20.3).

Build from master branch:

$ make -C test/ phaul-test
$ ls -alh test/phaul/phaul
-rwxr-xr-x. 1 root root 7.3M May 20 09:12 test/phaul/phaul

Build from fix-proto-gen branch:

$ make -C test/ phaul-test
$ ls -alh test/phaul/phaul
-rwxr-xr-x. 1 root root 7.1M May 20 09:16 test/phaul/phaul

@adrianreber
Copy link
Member

Thanks. So this change brings small improvements. Good.

@adrianreber adrianreber merged commit 748b90b into checkpoint-restore:master May 20, 2023
12 of 14 checks passed
@snprajwal snprajwal deleted the fix-proto-gen branch May 20, 2023 08:41
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.

None yet

3 participants