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

Update scripts after reorganization of the code #1064

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

rbehjati
Copy link
Contributor

@rbehjati rbehjati commented Jun 2, 2020

Ref #971, #1045

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

@rbehjati rbehjati added the WIP Work in progress label Jun 2, 2020
@rbehjati rbehjati force-pushed the format branch 7 times, most recently from d08a7ca to cc3c174 Compare June 3, 2020 12:41
@rbehjati rbehjati changed the title Format and test Update scripts after reorganization of the code Jun 3, 2020
Copy link
Contributor Author

@rbehjati rbehjati left a comment

Choose a reason for hiding this comment

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

The following scripts are updated, to enable quality check of the extracted crates, in particular the examples workspace:
format
check_formatting
run_tests
check_cargo_deny

Any thing else that should be updated?
The cloudbuild is taking a bit longer after this change. Is this something to worry about?

@@ -12,7 +12,9 @@ crate-type = ["cdylib"]
log = "*"
oak = "=0.1.0"
prost = "*"
rustfmt-nightly = "*"
rustfmt-nightly = "=1.4.14"
rustc-ap-rustc_session = "=654.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without these, Cargo uses version 659.0.0 and fails with:

error[E0407]: method `forward_checked` is not a member of trait `std::iter::Step`
  --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-rustc_span-659.0.0/def_id.rs:11:1
   |
11 | / rustc_index::newtype_index! {
12 | |     pub struct CrateId {
13 | |         ENCODABLE = custom
14 | |     }
15 | | }
   | |_^ not a member of trait `std::iter::Step`
   |
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

Is there a better way to fix this? I tried the newest nightly Rust, but even that did not fix the problem IIRC.

Copy link
Contributor

@daviddrysdale daviddrysdale Jun 3, 2020

Choose a reason for hiding this comment

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

Personally, I think we should just delete the rustfmt example; it doesn't illustrate anything that's not covered in other examples, it keeps on causing these sort of problems (cf. #588, #594), and it's one of the main things preventing us from moving to the Rust stable channel (#969)

Copy link
Collaborator

Choose a reason for hiding this comment

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

As the original author of that example, I'm fine to get rid of it, especially if it gets in the way of other things. (it is also quite slow to compile) @daviddrysdale do you want to do the honours?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created an issue: #1075

@@ -7,3 +7,7 @@ readonly SCRIPTS_DIR="$(dirname "$0")"
source "$SCRIPTS_DIR/common"

cargo deny check
cargo deny --manifest-path=oak_abi/Cargo.toml check
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are these really needed?

scripts/format Outdated
cargo fmt --all

# Format the `oak_abi` and `oak_utils` crates.
cargo fmt --all --manifest-path=oak_abi/Cargo.toml
cargo fmt --all --manifest-path=oak_utils/Cargo.toml
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only these crates need to be formatted?
Should oak_runtime and oak_loader be formatted too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oak_runtime and oak_loader are still part of the core workspace, so cargo fmt --all covers them.

Copy link
Collaborator

@conradgrobler conradgrobler left a comment

Choose a reason for hiding this comment

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

LTGM.

It seems that there are still some formatiing issues that must be fixed in examples.

Copy link
Contributor

@daviddrysdale daviddrysdale left a comment

Choose a reason for hiding this comment

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

LGTM if Cloudbuild is happy.

@@ -12,7 +12,9 @@ crate-type = ["cdylib"]
log = "*"
oak = "=0.1.0"
prost = "*"
rustfmt-nightly = "*"
rustfmt-nightly = "=1.4.14"
rustc-ap-rustc_session = "=654.0.0"
Copy link
Contributor

@daviddrysdale daviddrysdale Jun 3, 2020

Choose a reason for hiding this comment

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

Personally, I think we should just delete the rustfmt example; it doesn't illustrate anything that's not covered in other examples, it keeps on causing these sort of problems (cf. #588, #594), and it's one of the main things preventing us from moving to the Rust stable channel (#969)

@@ -7,3 +7,7 @@ readonly SCRIPTS_DIR="$(dirname "$0")"
source "$SCRIPTS_DIR/common"

cargo deny check
cargo deny --manifest-path=oak_abi/Cargo.toml check
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put something in common to declare a Bash array or two holding the master list of workspaces and/or standalone crates, and loop through that array in all of these scripts? That way only one place should need changing for any future re-orgs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cough Rust runner cough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Thanks. Done!

@@ -7,3 +7,7 @@ readonly SCRIPTS_DIR="$(dirname "$0")"
source "$SCRIPTS_DIR/common"

cargo deny check
cargo deny --manifest-path=oak_abi/Cargo.toml check
Copy link
Collaborator

Choose a reason for hiding this comment

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

cough Rust runner cough

* Updated `format` and `check_formatting` scripts to cover crates that
  are now excluded from the core workspace
* Updated `run_tests` and `check_cargo_deny` scripts
@rbehjati rbehjati merged commit 474e890 into project-oak:master Jun 3, 2020
@rbehjati rbehjati deleted the format branch June 3, 2020 19:55
@github-actions
Copy link

github-actions bot commented Jun 3, 2020

Reproducibility Index:

e01a0c7a158c80aaeb9607074136cd27e79f928e7bdacc26b87e43c9897bb899  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
598963d206ff76d09c097e26751a2edaa1ae4e2af4511492a81c96af6d2d709a  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
2d41478926ccfd06eb85bcbfcf15ea48eaac00ac617b076647d15b62ba1ed2aa  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
a4711ee09b5e1d69544399817347aa3ecee3db9fc2a52ccf51aa1809b561f933  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
8c8400f3481852b9621f7037797719c2bfd31ce0a53bbadda26425afeedd11e5  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
9bd3c9b905a94bb7b569ab44de8f4a2b4c6549282dc5a4c5d813872fdcfe370f  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
fff1590aa047d129986320a2496dc0497c48e245f0b6b97f9cc238da6bd8c64b  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
94b4a6d87af351d1d7cf1f25d78cbf7ed45e5bcd8d93a82e5f2bb7162985a416  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
fb161dd6905c8052b87b2f03884b0af58ad01b11b56ed0b626a6108c1ed98265  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
bb4f8b965283dbff96c023e8b2265e9594903a3d3ee038705f046c9bedabdc9a  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
bb1c2f869a9095976073598a86e3da8d322d92a1b35faf0063f69009b4942dd3  ./target/x86_64-unknown-linux-musl/release/oak_loader

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index c308422..45548c7 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,11 +1,11 @@
-888dde607bfb080fbb19acb2d757c5a7a8dd14c279bc98eb68c152888a253ff5  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
-aa5df52684fc21571484dcbc9a50955fe26c82a1d05e2b15d2913ac6c540025d  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
-8bc7b1323463c608b9dc9bac8bd62bbb5a0c6baa320df1c7c581cd127da90489  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
-2ddf099c23875f7dce73e41c19dfc2aa9fe3572a795e4e6b96b0356a98853aa5  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
-02f986b5334c5d1dae0902387a06a91caa04ee4ba988ab6d1843f75debec0d36  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
-53414a50383558bce1617b1b752961d75ce3b744664c2c48a7cd79fd856c8ee2  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
-19ab3ad7f0b15c7871461a31d73d3e7568315f1d50d59d0a2b37c87d3ffd03dc  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
-07d7ce6b796738666d35e961a4dd4ee92c79f61b77f8c7c797ca7b9e35868f66  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
-ca42ef3a20ec556bdc5e791971233d7801914b4d03fe7738c5155af7f84e5faa  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
-72c19cbd903d963ee1b3f9dda23e4e1528ee2dff304fae03ef51b0ba80fa62f7  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
+e01a0c7a158c80aaeb9607074136cd27e79f928e7bdacc26b87e43c9897bb899  ./examples/target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
+598963d206ff76d09c097e26751a2edaa1ae4e2af4511492a81c96af6d2d709a  ./examples/target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
+2d41478926ccfd06eb85bcbfcf15ea48eaac00ac617b076647d15b62ba1ed2aa  ./examples/target/wasm32-unknown-unknown/release/aggregator.wasm
+a4711ee09b5e1d69544399817347aa3ecee3db9fc2a52ccf51aa1809b561f933  ./examples/target/wasm32-unknown-unknown/release/chat.wasm
+8c8400f3481852b9621f7037797719c2bfd31ce0a53bbadda26425afeedd11e5  ./examples/target/wasm32-unknown-unknown/release/hello_world.wasm
+9bd3c9b905a94bb7b569ab44de8f4a2b4c6549282dc5a4c5d813872fdcfe370f  ./examples/target/wasm32-unknown-unknown/release/machine_learning.wasm
+fff1590aa047d129986320a2496dc0497c48e245f0b6b97f9cc238da6bd8c64b  ./examples/target/wasm32-unknown-unknown/release/private_set_intersection.wasm
+94b4a6d87af351d1d7cf1f25d78cbf7ed45e5bcd8d93a82e5f2bb7162985a416  ./examples/target/wasm32-unknown-unknown/release/running_average.wasm
+fb161dd6905c8052b87b2f03884b0af58ad01b11b56ed0b626a6108c1ed98265  ./examples/target/wasm32-unknown-unknown/release/translator.wasm
+bb4f8b965283dbff96c023e8b2265e9594903a3d3ee038705f046c9bedabdc9a  ./examples/target/wasm32-unknown-unknown/release/trusted_information_retrieval.wasm
 bb1c2f869a9095976073598a86e3da8d322d92a1b35faf0063f69009b4942dd3  ./target/x86_64-unknown-linux-musl/release/oak_loader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants