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

Create a "must have list" for crates in rust-vmm #14

Closed
6 tasks
andreeaflorescu opened this issue Feb 8, 2019 · 19 comments
Closed
6 tasks

Create a "must have list" for crates in rust-vmm #14

andreeaflorescu opened this issue Feb 8, 2019 · 19 comments

Comments

@andreeaflorescu
Copy link
Member

andreeaflorescu commented Feb 8, 2019

From my experience, it is very hard to get the time to work on improvements on the code once you have a working solution. That's why I think we should aim on having crates in a good shape before publishing them on crates.io.

I am thinking about having the following as requirements before publishing the crate:

  • High level documentation about the crate (what is it suppose to do, how can it be used in other projects, usage examples).
  • Mandatory documentation for public functions. To make sure we don't miss anything by mistake we can enforce this rule by using #![deny(missing-docs)].
  • Unit tests
  • Integration tests. For the kvm ioctls crate I've written some basic integration tests in python (because it is super easy, no judgement pls) for running cargo build, cargo test, cargo fmt (coding style), cargo clippy (linter) and I am also planning to add kcov as I think it is important to have adaptive coverage to make sure we keep a high quality bar for our crates. I think we should run the integration tests on each PR.
  • README.md
  • LICENSE

Feel free to add more things. We can also adjust the list if you think it is too much.

@andreeaflorescu
Copy link
Member Author

Once we decide on the list, we should update the community readme to specify what are the requirements before publishing a crate.

@andreeaflorescu
Copy link
Member Author

CC: @mcastelino @sameo @rbradford @jiangliu @zachreizner @sboeuf @dgreid

Can you point this issue to other people that might be interested?

@sameo
Copy link

sameo commented Feb 11, 2019

On integration tests, I think we should have a dedicated CI repository. A new crate will come with its own unit tests and should ideally add specific integration and functional tests into the CI repository. Any change to existing crates or adding new crates should trigger a new CI cycle, and it would be blocking the merge process.

Technically, all of this would be far simpler if we'd have one single repo for all crates and our CI, but I think it may be possible to run CI cycles from a separate repo. @sboeuf We have something similar with Kata, although we call the external ci repo from the runtime repo itself, right?

@andreeaflorescu
Copy link
Member Author

I think the integration tests should be in the repository, while the CI logic should probably be shared by all crates.

@sameo
Copy link

sameo commented Feb 11, 2019

I think the integration tests should be in the repository, while the CI logic should probably be shared by all crates.

So we'd expect any crate to provide a set of integration tests that would comply with the CI logic, i.e. should follow a sort of RPC that the CI could call into and get formatted result out of it. That should be doable and would make the crates more self contained for sure.

@sameo
Copy link

sameo commented Feb 11, 2019

Would any license be acceptable?

@andreeaflorescu
Copy link
Member Author

I find it super easy with pytest as it times each integration tests and pretty prints everything. Example from the kvm ioctls crate that I am working on:

pytest -vv
=============================================================================================================================== test session starts ===============================================================================================================================
platform linux -- Python 3.6.8, pytest-3.8.0, py-1.6.0, pluggy-0.7.1 -- /usr/bin/python3.6
cachedir: .pytest_cache
rootdir: /home/local/ANT/fandree/sources/work/rust-vmm/kvm-something-something/tests, inifile:
collected 4 items                                                                                                                                                                                                                                                                 

test_build.py::test_build PASSED                                                                                                                                                                                                                                            [ 25%]
test_build.py::test_build_musl PASSED                                                                                                                                                                                                                                       [ 50%]
test_build.py::test_style PASSED                                                                                                                                                                                                                                            [ 75%]
test_build.py::test_unittests PASSED                                                                                                                                                                                                                                        [100%]

============================================================================================================================ 4 passed in 0.23 seconds =============================================================================================================================

We can have a custom directory for integration tests that each crate must have. We can use integration_tests because tests is used by rust to find integration tests in the crate.

Then it would be as simple as calling pytest repo_name/tests -vv and pytest will take care of everything else.

@sameo
Copy link

sameo commented Feb 20, 2019

About licensing: The proposal would be to strongly recommend Apache 2. Or at least an OSI approved license.

@ericho
Copy link

ericho commented Mar 19, 2019

A couple of years ago the Rust community had an initiative called Rust libz blitz which consisted on verify a checklist of requirements that a crate should have, like this API guidelines, documentation and examples.

Maybe the whole checklist is too much for an early stage but it might help as a guideline on crate requirements.

@bonzini
Copy link
Member

bonzini commented Mar 26, 2019

About licensing: Apache 2 and GPLv2 are incompatible, so it wouldn't be possible to use rust-vmm in QEMU. What about just MIT?

@mswilson
Copy link

@bonzini, would it be acceptable to explicitly dual license "Apache License, Version 2.0 or MIT"?

@bonzini
Copy link
Member

bonzini commented Mar 27, 2019

@mswilson Of course, though that would be effectively the same as just MIT.

@mswilson
Copy link

mswilson commented Mar 27, 2019 via email

@bonzini
Copy link
Member

bonzini commented Mar 27, 2019

Sounds good to me!

@sameo
Copy link

sameo commented Mar 27, 2019

Sounds good to me as well.

@jiangliu
Copy link
Member

sounds good to me as well.

@ktian1
Copy link

ktian1 commented Apr 11, 2019

what about versioning scheme? I saw @andreeaflorescu plans to publish v0.1.0 of kvm-ioctl crate. It might be good to have a common guideline for all rust-vmm crates...

@andreeaflorescu
Copy link
Member Author

@ktian1 That's a great point and we should have it written somewhere. My proposal up to the point we want to release a 1.0.0 version is to have patch versions for non-breaking changes & bug fixes and minor versions for breaking changes. This comes with the disadvantage that we are not following the general accepted versioning of packages, but I don't really have a better idea there. Any other suggestions?

sameo pushed a commit to sameo/linux-loader that referenced this issue May 6, 2019
rust-vmm/community#14

Add the license files meanwhile.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
bjzhjing added a commit to bjzhjing/linux-loader that referenced this issue May 8, 2019
rust-vmm/community#14

Add the license files meanwhile.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
sameo pushed a commit to bjzhjing/linux-loader that referenced this issue Jun 7, 2019
rust-vmm/community#14

Add the license files meanwhile.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
rbradford pushed a commit to rust-vmm/linux-loader that referenced this issue Jun 13, 2019
rust-vmm/community#14

Add the license files meanwhile.

Signed-off-by: Cathy Zhang <cathy.zhang@intel.com>
@andreeaflorescu
Copy link
Member Author

The guidelines are now part of the community readme.
Created a separate issue regarding the version of crates #68

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

No branches or pull requests

7 participants