Wrap Rust `protoc` in Oak utils #533

ipetr0v · 2020-01-28T12:37:31Z

This change:

Adds oak_utils crate, that runs protoc/protoc_grpc and checks updated files
Updates build.rs files to use oak_utils

Fixes #510

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.

ipetr0v · 2020-01-30T13:32:44Z

protoc-rust-grpc was updated due to these errors: #510 (comment)

tiziano88 · 2020-01-30T20:03:44Z

third_party/grpc-rust/protoc-rust-grpc/src/lib.rs

@@ -68,7 +67,7 @@ pub fn run(args: Args) -> Result<()> {

    let mut includes = args.includes;
    if includes.is_empty() {
-        static DOT_SLICE: &'static [&'static str] = &["."];
+        static DOT_SLICE: &[&str] = &["."];


These changes were just introduced in #531 , why do they need to be reverted now?
/cc @blaxill

I did it due to this error: #510 (comment)
that started appearing after I added a dependency (not a build-dependency) on a protoc-rust-grpc

Ah interesting, so it seems that these changes were made by us to satisfy clippy. @blaxill do you remember why it was necessary to revert them?

I just pulled from upstream, the important change was the less restrictive Cargo.toml 2.8 -> 2 change, so that it could generate 2.10 protoc files similar to the other generated proto files.

Why did nothing break when these changes were undone though?

It didn't get "clippy'ed" as a build/dev dependency, but since we are wrapping it it is a full dependency of oak_utils and so is clippy'ed now.

tiziano88 · 2020-01-31T11:05:12Z

docs/programming-oak.md

- [Persistent Storage](#persistent-storage)
- [Testing](#testing)
-  - [Testing Multi-Node Applications](#testing-multi-node-applications)
+- [Programming Oak](#programming-oak)


note it is not common to include the title in the TOC, or was this generated by some automated tool?

I didn't touch this file, it was changed by the format.

From within Docker? But why did it change it now?

Fwiw [Programming Oak](#programming-oak) is an addition, perhaps from a merge or by accident?

I'll revert this change then

Oh, I get it
This change is automatically applied by VSCode, when I save the file.

sdk/rust/oak_utils/src/lib.rs

tiziano88 · 2020-02-03T14:05:31Z

third_party/grpc-rust/protoc-rust-grpc/src/lib.rs

@@ -68,7 +67,7 @@ pub fn run(args: Args) -> Result<()> {

    let mut includes = args.includes;
    if includes.is_empty() {
-        static DOT_SLICE: &'static [&'static str] = &["."];
+        static DOT_SLICE: &[&str] = &["."];


Why did nothing break when these changes were undone though?

sdk/rust/oak_utils/src/lib.rs

sdk/rust/oak_utils/src/tests.rs

tiziano88 · 2020-02-03T20:42:20Z

sdk/rust/oak_utils/src/lib.rs

+
+// This function returns a list of files in the `new_dir` that are different from files with
+// same names in the `old_dir`.
+fn get_changed_and_removed_files(old_dir: &Path, new_dir: &Path) -> (Vec<String>, Vec<String>) {


I think the return type and the logic are getting a bit too complicated.

Perhaps another solution could be:

collect filenames from both maps in a single set

iterate over the set, and get the value from both the old and new map (as options)

collect the old and new option types in a struct like:

struct FileDiff { old_content: Option<String>, new_content: Option<String>, }

add a method on FileDiff::action() to figure out what to do about the file, perhaps it should return one value of some enum:

enum FileAction { Ignore, Update{ new_content: String }, Remove`, }

apply the necessary change based on this enum value in a final step.

WDYT? I think it should be more readable, and kind of fun to implement in a "functional" style, but if you don't think that's worth it feel free to submit as it is (or do it in a future PR).

Will be fixed in #548

tiziano88 · 2020-02-03T20:44:59Z

sdk/rust/oak_utils/src/lib.rs

+
+// This function returns a list of files in the `new_dir` that are different from files with
+// same names in the `old_dir`.
+fn get_changed_and_removed_files(old_dir: &Path, new_dir: &Path) -> (Vec<String>, Vec<String>) {


I think this function would be much easier to test if it took as input the corresponding maps, instead of figuring them out from the actual file system. Then the unit tests would look trivial, since you can just pass the maps without having to create any temp files. Up to you anyways.

Will be fixed in #548

daviddrysdale · 2020-02-12T14:42:11Z

I don't think this is cleaning up its temporary directories properly – I've got about 10GiB of /tmp subdirectories that look to be produced by this.

ipetr0v · 2020-02-12T15:28:46Z

I don't see temporary directories on my machine and inside Docker.
Could you please send directory names?

daviddrysdale · 2020-02-12T15:55:51Z

~/src/oak{master}:ls -d /tmp/*/debug
ls: cannot access '/tmp/*/debug': No such file or directory
~/src/oak{master}:cd examples/hello_world/module/
~/src/oak/examples/hello_world/module{master}:cargo test > /dev/null 2>&1
~/src/oak/examples/hello_world/module{master}:ls -d /tmp/*/debug
/tmp/k2aaC3wmhUuB/debug  /tmp/Wcd3O4sir75w/debug  /tmp/ZUzEvTgIe9Eg/debug
~/src/oak/examples/hello_world/module{master}:du -m -s /tmp/k2aaC3wmhUuB /tmp/Wcd3O4sir75w /tmp/ZUzEvTgIe9Eg
459	/tmp/k2aaC3wmhUuB
355	/tmp/Wcd3O4sir75w
531	/tmp/ZUzEvTgIe9Eg

blaxill · 2020-02-12T15:59:53Z

@daviddrysdale this likely could this be #578 switching to not controlling out-dir, and parsing cargo json for the cargo defined directory may fix it.

daviddrysdale · 2020-02-12T16:37:28Z

Ah, that makes more sense – sorry for the false alarm @ipetr0v.

googlebot added the cla: yes label Jan 28, 2020

ipetr0v changed the title ~~Oak utils~~ Wrap Rust protoc in Oak utils Jan 28, 2020

ipetr0v added the WIP Work in progress label Jan 28, 2020

ipetr0v marked this pull request as ready for review January 28, 2020 14:33

ipetr0v added 8 commits January 28, 2020 14:34

Add oak_utils

695e341

Update oak_utils

3a5f836

Use oak_utils

b068439

Updated comments

0a61ede

Add protoc_grpc

daa6b48

Add generated files

5a1f338

Add docs

66862f0

Update generated files

71a4c91

ipetr0v force-pushed the oak_utils branch from 2c7f519 to 71a4c91 Compare January 28, 2020 14:39

ipetr0v added 4 commits January 28, 2020 16:10

Update generated files

af92d15

Revert Cargo.lock files

dbe7a81

Move oak_utils

f5798c9

Update Cargo.toml

fa803ee

tiziano88 mentioned this pull request Jan 28, 2020

Single Cargo workspace for the entire repository #534

Closed

ipetr0v added 4 commits January 29, 2020 11:06

Update Cargo.lock

059de25

Fix formatting

c26e714

Fix oak_utils name

232657b

Update Cargo.lock

788d625

ipetr0v added 3 commits January 30, 2020 13:33

Update protoc-rust-grpc

05d3576

Delete Default from protoc-rust-grpc

910c350

Delete tests

62c221c

ipetr0v requested review from tiziano88 and daviddrysdale January 30, 2020 14:40

ipetr0v removed the WIP Work in progress label Jan 30, 2020

tiziano88 reviewed Jan 31, 2020

View reviewed changes

Fix after review

2932283

ipetr0v requested a review from tiziano88 January 31, 2020 17:45

Fix docs

bbbbd06

tiziano88 reviewed Feb 3, 2020

View reviewed changes

ipetr0v added 3 commits February 3, 2020 17:27

Fix after review

7b5bfdb

Update oak_utils

15d1d03

Update Cargo.lock files

ade5ee0

ipetr0v requested a review from tiziano88 February 3, 2020 18:33

tiziano88 reviewed Feb 3, 2020

View reviewed changes

ipetr0v added the WIP Work in progress label Feb 4, 2020

ipetr0v requested a review from tiziano88 February 4, 2020 12:00

ipetr0v mentioned this pull request Feb 4, 2020

Refactor oak_utils #548

Closed

tiziano88 approved these changes Feb 4, 2020

View reviewed changes

ipetr0v merged commit dfb8ff7 into project-oak:master Feb 4, 2020

ipetr0v deleted the oak_utils branch February 4, 2020 12:06

blaxill mentioned this pull request Feb 12, 2020

Collect garbage when generating Wasm files #601

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap Rust `protoc` in Oak utils #533

Wrap Rust `protoc` in Oak utils #533

ipetr0v commented Jan 28, 2020 •

edited

Loading

ipetr0v commented Jan 30, 2020 •

edited

Loading

tiziano88 Jan 30, 2020

ipetr0v Jan 31, 2020

tiziano88 Jan 31, 2020

blaxill Jan 31, 2020

tiziano88 Feb 3, 2020

blaxill Feb 3, 2020

tiziano88 Jan 31, 2020

ipetr0v Jan 31, 2020

tiziano88 Jan 31, 2020

blaxill Jan 31, 2020

ipetr0v Feb 3, 2020

ipetr0v Feb 3, 2020

tiziano88 Feb 3, 2020

tiziano88 Feb 3, 2020

ipetr0v Feb 4, 2020

tiziano88 Feb 3, 2020

ipetr0v Feb 4, 2020

daviddrysdale commented Feb 12, 2020

ipetr0v commented Feb 12, 2020 •

edited

Loading

daviddrysdale commented Feb 12, 2020

blaxill commented Feb 12, 2020

daviddrysdale commented Feb 12, 2020

Wrap Rust protoc in Oak utils #533

Wrap Rust protoc in Oak utils #533

Conversation

ipetr0v commented Jan 28, 2020 • edited Loading

Checklist

ipetr0v commented Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale commented Feb 12, 2020

ipetr0v commented Feb 12, 2020 • edited Loading

daviddrysdale commented Feb 12, 2020

blaxill commented Feb 12, 2020

daviddrysdale commented Feb 12, 2020

Wrap Rust `protoc` in Oak utils #533

Wrap Rust `protoc` in Oak utils #533

ipetr0v commented Jan 28, 2020 •

edited

Loading

ipetr0v commented Jan 30, 2020 •

edited

Loading

ipetr0v commented Feb 12, 2020 •

edited

Loading