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

Make youki a library crate #403

Merged
merged 11 commits into from
Oct 23, 2021
Merged

Make youki a library crate #403

merged 11 commits into from
Oct 23, 2021

Conversation

Furisto
Copy link
Collaborator

@Furisto Furisto commented Oct 21, 2021

As discussed in #396, this creates a new crate libcontainer, the youki binary itself will only be a thin wrapper around this library. All crates have been moved under the crate folder, so that we now have the following structure

crates
- integration_test
- libcgroups
- libcontainer
- libseccomp
- test-framework
docs
hacks
...

Despite my comments in #396, I have now added a lib prefix as paths like container::container::Container, not only look weird, but also confused the compiler.
No that we have one cargo.lock file, there was a problem with multiple versions of clap incompatible to each other, so I had to settle on clap 3.0.0.0-beta.2, beta.3 and beta.4 did not work for the integration test, and with beta.5 the compiler complained that the macro resolution got so complicated that the compilation had to be aborted. That also means that I had to remove "forbid_empty_values" from the command args as it is not available in this version. Besides this, there should not be any functional changes.

@Furisto Furisto force-pushed the libcrate branch 2 times, most recently from 9cc83e2 to b865a6b Compare October 21, 2021 20:52
@Furisto Furisto marked this pull request as ready for review October 21, 2021 21:42
@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

Merging #403 (52a262f) into main (017feba) will decrease coverage by 9.30%.
The diff coverage is 21.42%.

@@            Coverage Diff             @@
##             main     #403      +/-   ##
==========================================
- Coverage   76.62%   67.31%   -9.31%     
==========================================
  Files          53       76      +23     
  Lines        8487     9746    +1259     
==========================================
+ Hits         6503     6561      +58     
- Misses       1984     3185    +1201     

Copy link
Collaborator

@yihuaf yihuaf left a comment

Choose a reason for hiding this comment

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

LGTM. Agree that adding lib prefix is good. We can resolve clap versioning issue with another PR if needed. Having the same version across all of our code can be a good thing. Sorry about the small merge conflicts. Should be easy to resolve.

I noted that the coverage dropped by 9+%. Likely this is due to adding integration tests. Writing unit tests for integration tests would be weird and I don't if it make sense. May be we should look into ignoring integration test crate when it comes to coverage? Again, don't have to be resolved in this PR.

@yihuaf
Copy link
Collaborator

yihuaf commented Oct 22, 2021

Also may be wait for @utam0k to comment/approve this as well.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 22, 2021

Hey, How about we keep the test framework and integration tests separate from the core libs? When we need to use the code from core lib in tests, we can make them a dependency by path. This will solve the issue of clap version, as well as we can easily separate out the tests in their own repo later, similar to oci-validation tools. Also this will solve the code coverage issue. I don't feel that making the integration tests part of core libs / appliaction is a good idea, also given that we have to remove the forbid_empty_value for it to work.
This will make the layout as :

youki_integration_tests
- integration_test
- test-framework
crates
- libcgroups
- libcontainer
- libseccomp
docs
hacks
...

At least personally I feel this will be better than combining integration tests and core libs together, and the resulting possible dependency issues.

@yihuaf
Copy link
Collaborator

yihuaf commented Oct 22, 2021

By default integration test should be part of the same workspace with Youki and share the dependencies.I don't like inside one repo we have multiple rust workspaces. Currently integration test is in its own rust workspace, but I am not sure if there is any specific reason that we have to do it. For clap version conflict, we should fix the our code instead of having to use different clap version to resolve the issue. The youki/crates/* includes all of the crates, not just core libs. For example, crates/youki contain the binary crate with the main function.

@Furisto
Copy link
Collaborator Author

Furisto commented Oct 22, 2021

I would like to keep everything within one workspace as well. We could move test specific crates to a test folder within the same workspace, that would make it easier to exclude them from code coverage.

@Furisto
Copy link
Collaborator Author

Furisto commented Oct 22, 2021

Branch is mergable again. Would appreciate it, if we could hold of merging other PRs until we have wrapped this up.

@utam0k
Copy link
Member

utam0k commented Oct 23, 2021

Do you use the workspace feature?

@yihuaf
Copy link
Collaborator

yihuaf commented Oct 23, 2021

Do you use the workspace feature?

He did, with:

[workspace]
members = [
    "crates/*"
]

@Furisto
Copy link
Collaborator Author

Furisto commented Oct 23, 2021

@utam0k Is this change ok from your side now?

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

looks good!

@utam0k utam0k merged commit 9827ac4 into youki-dev:main Oct 23, 2021
@YJDoc2 YJDoc2 mentioned this pull request Oct 27, 2021
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.

5 participants