Skip to content
This repository was archived by the owner on Jul 16, 2020. It is now read-only.

Conversation

@mbestavros
Copy link

To enforce proper formatting standards on Enarx projects, we'd like to run cargo fmt on all incoming pull requests to Enarx projects. This enables the appropriate test using Travis CI.

Resolves #11.

@mbestavros mbestavros force-pushed the travis-formatting branch 8 times, most recently from 2039b32 to 54d32ae Compare September 13, 2019 13:11
@mbestavros
Copy link
Author

I've amended the tests to also run in the amd-sev subdirectory. That crate isn't properly formatted yet, which is why tests are failing. I can roll the formatting into this patch, or make a separate one if preferred.

@npmccallum
Copy link
Contributor

One pull request with two commits please.

@npmccallum
Copy link
Contributor

I can't believe I missed the obvious solution to all this. Make the parent directory a Cargo workspace.

https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html

@mbestavros
Copy link
Author

Agreed, that is the cleanest way to get what we need. How should I approach making that change? Separate commit adding the workspace before adding travis.yml?

@npmccallum
Copy link
Contributor

Yup. Logical commits.

@mbestavros mbestavros force-pushed the travis-formatting branch 9 times, most recently from 9ac8ce9 to 4cf0cd4 Compare September 17, 2019 19:49
@mbestavros mbestavros force-pushed the travis-formatting branch 2 times, most recently from 8423d83 to 938b6ab Compare September 30, 2019 18:50
@mbestavros mbestavros force-pushed the travis-formatting branch 3 times, most recently from e58ab99 to ff6854e Compare October 7, 2019 18:22
@mbestavros
Copy link
Author

@steveej I've reworded most of the commits; they should match up with the logical changes in each of them better now.

@mbestavros mbestavros force-pushed the travis-formatting branch 5 times, most recently from adb2e24 to 44d2632 Compare October 8, 2019 14:48
steveej
steveej previously approved these changes Oct 8, 2019
Copy link
Contributor

@steveej steveej left a comment

Choose a reason for hiding this comment

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

This looks good to me now, thank you!

Copy link
Contributor

@steveej steveej left a comment

Choose a reason for hiding this comment

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

I'm having second thoughts on my previous approval, specifically because the generated lockfile has to generalize the dependencies isn't guaranteed to be consistent with all of the member crate lockfiles, or I don't understand the consistency here ;-) The issue I'm seeing is that the generated lockfile can bump dependencies which are used in tests from the root of the workspace, which are different from the one inside the members.

@steveej steveej self-requested a review October 8, 2019 17:11
@mbestavros mbestavros force-pushed the travis-formatting branch 2 times, most recently from 52d880d to 96c59a7 Compare October 8, 2019 19:08
Copy link
Contributor

@steveej steveej left a comment

Choose a reason for hiding this comment

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

I have made change to the ketuvim (enarx-archive/ketuvim#8) so that it re-exports the sev crate. This alleviates the following issue, which arises when pinning sev in ketuvim to a different version than it's using internally:

error[E0308]: mismatched types
   --> amd-sev/src/main.rs:131:19
    |
131 |     launch.inject(secret, addr, len).unwrap();
    |                   ^^^^^^ expected struct `sev::launch::Secret`, found a different struct `sev::launch::Secret`
    |
    = note: expected type `sev::launch::Secret` (struct `sev::launch::Secret`)
               found type `sev::launch::Secret` (struct `sev::launch::Secret`)
note: Perhaps two different versions of crate `sev` are being used?
   --> amd-sev/src/main.rs:131:19
    |
131 |     launch.inject(secret, addr, len).unwrap();

@mbestavros mbestavros force-pushed the travis-formatting branch 3 times, most recently from 1c58e91 to 107869b Compare October 9, 2019 14:35
Copy link
Contributor

@steveej steveej left a comment

Choose a reason for hiding this comment

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

I think this is the last one on the issue of reproducibility. After that we'll get to what cargo audit has to tell us.

Copy link
Contributor

@steveej steveej left a comment

Choose a reason for hiding this comment

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

Thanks for sticking through this! :-)

@steveej steveej merged commit 5b35d29 into enarx-archive:master Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a cargo fmt test to the repo

3 participants