[Rust] flatc build script wrapper #6453

vadixidav · 2021-02-08T20:07:07Z

This is for #5216. Currently a draft PR just for review purposes.

Co-authored-by: Geordon Worley <vadixidav@gmail.com>

google-cla · 2021-02-08T20:07:12Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

CasperN · 2021-02-08T23:49:14Z

rust/flatc/src/lib.rs

+/// [google/flatbuffers.git]: https://github.com/google/flatbuffers
+const FLATBUFFERS_COMMIT_SHA: &str = env!("FLATBUFFERS_COMMIT_SHA");
+
+/// The location of the copy of the `flatc` executable compiled by this crate.


I really like the idea of packaging flatc with this crate... I wonder if we can use it to couple the flatbuffers library with the flatc version somehow.

Related: #6149

@krojew might have opinions on this

We already are aiming at unifying versions across languages, so packaging flatc can be a nice feautre to have.

CasperN · 2021-02-08T23:52:01Z

rust/flatc/src/lib.rs

+const FLATC_EXECUTABLE: &str = concat!(env!("OUT_DIR"), "/bin/flatc");
+
+/// An internal error that occured.
+struct Error {


consider using this_error to ease the boilerplate

CasperN · 2021-02-08T23:54:41Z

tests/rust_usage_test/bin/monster_example.rs

@@ -1,16 +1,19 @@
 extern crate flatbuffers;

 #[allow(dead_code, unused_imports)]
-#[path = "../../include_test/include_test1_generated.rs"]
-pub mod include_test1_generated;
+pub mod include_test1_generated {


I've been using tests/rust_usage_test/outdir/ as a regression test for build.rs support, please extend that for your tests. I'm not sure if we should promote this over the #[path = ... ] approach just yet since that is a breaking change.

Also, the way imports and stuff work may be affected by the solution to #5589 in breaking ways too, and I'd like to minimize the amount of sequential breakage. (btw, if you have suggestions, please comment on that issue)

CasperN · 2021-02-08T23:55:48Z

tests/RustTest.sh

@@ -46,6 +57,15 @@ rustup component add clippy
 cargo clippy $TARGET_FLAG
 check_test_result "No Cargo clippy lints test"

+cargo test $TARGET_FLAG -- --quiet --test build_script_wrapper
+TEST_RESULT=$?


please use the check_test_result function

CasperN · 2021-02-08T23:58:45Z

rust/flatc/src/lib.rs

@@ -0,0 +1,279 @@
+/*
+ * Copyright 2018 Google Inc. All rights reserved.


CasperN · 2021-02-09T00:01:51Z

rust/flatc/build.rs

+
+fn main() {
+    let git_sha = if let Ok(output) = Command::new("git")
+        .args(&["rev-parse", "--short=7", "HEAD"])


no need to make it short imo

CasperN · 2021-02-09T00:08:36Z

rust/flatc/src/lib.rs

+    fn try_compile(&self) -> Result<(), Error> {
+        let output_path = self.output_path.as_ref().map_or_else(
+            || env::var("OUT_DIR").unwrap(),
+            |path| format!("{}", path.display()),


I think this can be done in new. the None variant isn't actually used

CasperN · 2021-02-09T00:12:32Z

tests/rust_usage_test/tests/build_script_test.rs

+fn build_script_wrapper() {
+    let output = Path::new(&env::var("OUT_DIR").unwrap()).join("monster_generated.rs");
+
+    let src = fs::read_to_string(&output).expect("Failed to read generated code");


might want to test that at least some known symbols/substrings are in there

CasperN · 2021-02-09T00:15:05Z

rust/flatc/src/lib.rs

+
+impl Build {
+    #[doc(hidden)]
+    pub fn output<P: AsRef<Path>>(&mut self, dir: P) -> &mut Self {


is this used?

vadixidav · 2021-02-09T17:08:51Z

Thank you for the review. This PR is mostly just what was written here: https://github.com/rjsberry/flatbuffers/tree/rust/flatc. I applied some changes on top of that and added myself to the commit as a co author. I will be adding a few more commits to help me get it working for my use case specifically first. After that, I will address the things in the review. I will circle back around to make broad improvements on this PR before fully opening it up.

google-cla · 2021-02-09T17:22:07Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

vadixidav · 2021-02-21T22:11:32Z

I ran into an issue where this intersected with my day job and it is going to take a long time to get legal to give me the okay because they are so slow. Therefore, I am going to have to pull out of this effort. Anyone feel free to rebase my branch and make the above fixed, and I would greatly appreciate it.

[Rust] flatc build script wrapper

4f3d9c4

Co-authored-by: Geordon Worley <vadixidav@gmail.com>

github-actions bot added the rust label Feb 8, 2021

vadixidav mentioned this pull request Feb 8, 2021

[Rust] flatc build script wrapper #5216

Closed

CasperN reviewed Feb 9, 2021

View reviewed changes

add a method to support --gen-all flag

96214d4

vadixidav closed this Feb 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rust] flatc build script wrapper #6453

[Rust] flatc build script wrapper #6453

vadixidav commented Feb 8, 2021

google-cla bot commented Feb 8, 2021

CasperN Feb 8, 2021

krojew Feb 9, 2021

CasperN Feb 8, 2021

CasperN Feb 8, 2021

CasperN Feb 8, 2021

CasperN Feb 8, 2021

CasperN Feb 9, 2021

CasperN Feb 9, 2021

CasperN Feb 9, 2021

CasperN Feb 9, 2021

vadixidav commented Feb 9, 2021 •

edited

Loading

google-cla bot commented Feb 9, 2021

vadixidav commented Feb 21, 2021

		@@ -0,0 +1,279 @@
		/*
		* Copyright 2018 Google Inc. All rights reserved.

[Rust] flatc build script wrapper #6453

[Rust] flatc build script wrapper #6453

Conversation

vadixidav commented Feb 8, 2021

google-cla bot commented Feb 8, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

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

vadixidav commented Feb 9, 2021 • edited Loading

google-cla bot commented Feb 9, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

vadixidav commented Feb 21, 2021

vadixidav commented Feb 9, 2021 •

edited

Loading