Rust 1.39 Update + Spring Cleaning by dhrgit · Pull Request #1419 · firecracker-microvm/firecracker

dhrgit · 2019-11-22T17:21:23Z

Reason for This PR

Tidy is good, tidy is love.

Description of Changes

workspace: all sources are now placed in their respective crates, under /src/<crate>;
workspace: unified the jailer source under a single bin crate /src/jailer; removed jailer dependency from the firecracker crate;
workspace: configured the build artifacts dir via .cargo/config;
devctr: removed env variable used to override the cargo build dir; updated to Rust toolchain 1.39;
clippy: added safety doc comments;
vsock: switched to using zeroed memory for the TX buffer, instead of mem::uninitialized, since the latter is both unsafe and deprecated by Rust 1.39;
also fixed up some whitespace errors.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria. Where there are two options, keep one.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
Either this PR is linked to an issue, or, the reason for this PR is
clearly provided.
The description of changes is clear and encompassing.
Either no docs need to be updated as part of this PR, or, the required
doc changes are included in this PR. Docs in scope are all *.md files
located either in the repository root, or in the docs/ directory.
Either no code has been touched, or, code-level documentation for touched
code is included in this PR.
Either no API changes are included in this PR, or, the API changes are
reflected in firecracker/swagger.yaml.
Either the changes in this PR have no user impact, or, the changes in
this PR have user impact and have been added to the CHANGELOG.md file.
Either no new unsafe code has been added, or, the newly added unsafe
code is unavoidable and properly documented.

andreeaflorescu · 2019-11-25T08:26:10Z

Changes LGTM. I'll approve this once the arm build is fixed as well.

acatangiu · 2019-11-25T10:12:02Z

Do you have a quick reference resource you can link that explains why moving the crates under /src is tidy while the current state is not?

andreeaflorescu · 2019-11-25T10:16:35Z

My personal take on this is I that it makes more sense as the source code is clearly separated from the documentation and other utilities. It is much easier to navigate through misc docs and tools as they don't mingle with rust code as well.

dhrgit · 2019-11-25T10:32:52Z

Do you have a quick reference resource you can link that explains why moving the crates under /src is tidy while the current state is not?

I don't have one handy. However, you can look at it like this: just as a crate is structured using a src dir for the sources, a target dir for the build artifacts, and the root for support files (such as Cargo.toml), so should a workspace be. I.e. sources go under src, integration tests go under tests, documentation goes under docs, and so on.

Also, in this particular case, it's more tidy because this arrangement enables decoupling the firecracker and jailer crates. In the previous layout, the jailer main was part of the firecracker crate, while the jailer core lived in its own crate. Now, they are two independent bin (executable) crates.

acatangiu · 2019-11-25T10:42:38Z

Makes sense. I thought we enable some Cargo magic as well (that we were previously missing).

src/devices/src/virtio/vsock/csm/txbuf.rs

raduweiss · 2019-11-25T17:29:02Z

My esthetics only opinion is "+100".

acatangiu

LGTM

dianpopa

I do like this workspace reorganization better than the older one 👍 However, given that this PR is changing in an impactful way how Firecracker will look from now on, I think the entire team should give their approval on this or at least be informed of it before merge in order not to be taken by surprise.

dianpopa · 2019-11-27T09:37:58Z

Cargo.toml

-
-[dev-dependencies]
-tempfile = ">=3.0.2"
+[workspace]


nit: your commit message misspells "workspace" twice (i.e. worspace)

dianpopa · 2019-11-27T09:53:39Z

tests/integration_tests/build/test_unittests.py

@@ -39,9 +39,6 @@ def test_unittests(test_session_root_path, target):
    if MACHINE == "x86_64":


This might be a miss as I do not think there is a reason why we would not want to test all features on all architectures.

A miss indeed. Fixed.

dianpopa · 2019-11-27T10:06:46Z

src/utils/src/structs.rs

+///
+/// This is unsafe because the structs are initialized to unverified data read from the input.
+/// `read_struct_slice` should only be called for plain old data structs.  It is not endian safe.
 // This lint check is now deprecated - https://github.com/rust-lang/rust-clippy/pull/3478/files


Is this comment still valid? What lint check is it refferring to?

Removed. Looks like that comment had been left behind when the clippy override was removed.

sandreim

I think this re-org is aesthetically pleasing. 👌

Tidied up the cargo workspace a bit: - all sources are now placed in their respective crates, under `/src/<crate>`; - unified the jailer sources under the self-contained `/src/jailer` bin crate; - fixed `utils/src/lib.rs` copyright + some nitty tidying up; - arch-gated the `cpuid` crate to x86_64; - fixed some whitespace errors. Signed-off-by: Dan Horobeanu <dhr@amazon.com>

- cargo: configured the build dir via `.cargo/config`; - devctr: removed env variable used to override build dir; updated rust toolchain to 1.39; - clippy: added safety doc comments; - vsock: switched to using zeroed memory for the TX buffer, instead of `mem::uninitialized`, since the latter is both unsafe and deprecated by Rust 1.39. Signed-off-by: Dan Horobeanu <dhr@amazon.com>

Fixed a bunch of whitespace errors scattered throughout the repo. Signed-off-by: Dan Horobeanu <dhr@amazon.com>

Some of our crates were pinning their dependency versions. This was redundant, since we are using a versioned, worskspace-wide, Cargo.lock which we update with every release. It also resulted in `cargo update` not updating the depended-upon crates to their latest versions. This commit switches all dependecy version specifications to the latest version (i.e. the ">=<version>" syntax), and also updates Cargo.lock (via `cargo update`). Signed-off-by: Dan Horobeanu <dhr@amazon.com>

- Rust 1.35 cannot parse Firecracker's Cargo.lock somehow - While Rust's latest release is 1.40.0, we'd like to keep in sync with Firecracker upstream. firecracker-microvm/firecracker#1419 Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

dhrgit self-assigned this Nov 22, 2019

dhrgit force-pushed the cleanup-139 branch 2 times, most recently from d9a225a to 42a1bf9 Compare November 24, 2019 16:30

acatangiu reviewed Nov 25, 2019

View reviewed changes

src/devices/src/virtio/vsock/csm/txbuf.rs Show resolved Hide resolved

acatangiu previously approved these changes Nov 26, 2019

View reviewed changes

dhrgit dismissed acatangiu’s stale review via f1d7bb5 November 26, 2019 13:17

dhrgit force-pushed the cleanup-139 branch from 42a1bf9 to f1d7bb5 Compare November 26, 2019 13:17

andreeaflorescu previously approved these changes Nov 26, 2019

View reviewed changes

dianpopa reviewed Nov 27, 2019

View reviewed changes

sandreim self-requested a review November 27, 2019 11:06

sandreim reviewed Nov 27, 2019

View reviewed changes

dhrgit dismissed andreeaflorescu’s stale review via 2e67394 November 27, 2019 11:40

dhrgit force-pushed the cleanup-139 branch from f1d7bb5 to 2e67394 Compare November 27, 2019 11:40

dianpopa previously approved these changes Nov 27, 2019

View reviewed changes

andreeaflorescu previously approved these changes Nov 27, 2019

View reviewed changes

dhrgit dismissed stale reviews from andreeaflorescu and dianpopa via e9d4b06 November 27, 2019 17:46

dhrgit added 4 commits November 28, 2019 10:40

Fixed a bunch of whitespace errors

47c1ade

Fixed a bunch of whitespace errors scattered throughout the repo. Signed-off-by: Dan Horobeanu <dhr@amazon.com>

dhrgit force-pushed the cleanup-139 branch from e9d4b06 to 19f7361 Compare November 28, 2019 10:42

dianpopa approved these changes Nov 28, 2019

View reviewed changes

acatangiu approved these changes Nov 29, 2019

View reviewed changes

acatangiu merged commit 777e366 into firecracker-microvm:master Nov 29, 2019

		@@ -39,9 +39,6 @@ def test_unittests(test_session_root_path, target):
		if MACHINE == "x86_64":

Conversation

dhrgit commented Nov 22, 2019

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Uh oh!

andreeaflorescu commented Nov 25, 2019

Uh oh!

acatangiu commented Nov 25, 2019

Uh oh!

andreeaflorescu commented Nov 25, 2019

Uh oh!

dhrgit commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acatangiu commented Nov 25, 2019

Uh oh!

Uh oh!

raduweiss commented Nov 25, 2019

Uh oh!

acatangiu left a comment

Choose a reason for hiding this comment

Uh oh!

dianpopa left a comment

Choose a reason for hiding this comment

Uh oh!

dianpopa Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

dhrgit Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

dianpopa Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

dhrgit Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

dianpopa Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

dhrgit Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

sandreim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dhrgit commented Nov 25, 2019 •

edited

Loading