You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/04/23 11:27:20 UTC
[GitHub] [arrow] nevi-me opened a new pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
nevi-me opened a new pull request #7018:
URL: https://github.com/apache/arrow/pull/7018
When a user compiles the `flight` crate, a `build.rs` script is invoked. This script recursively looks for the `format/Flight.proto` path. A user might not have that path, as they would not have cloned the arrow repository, and as such, the build fails.
This change:
a) checks if the `../../format/Flight.proto` path exists, and only builds if the file exists, and has been changed.
b) checks in the proto file into the `src/` directory, changing the default location from an opaque directory in the `target/{debug|release}/build-xxx` folder.
The rationale for checking in the proto file is that if the file is not rebuilt, `flight` users are still able to access the file. It's also beneficial for users to easily view the file, and better understand how the generated code looks like.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kyprifog edited a comment on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
kyprifog edited a comment on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-661894115
Sorry I wasn't being clear, I was trying to create a fork of the ballista example by including the ballista crate (v 0.2.5) which was throwing this error on building arrow-flight 0.17.0, but looking at the existing rust example I can see how that won't work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on issue #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on issue #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-618349230
https://issues.apache.org/jira/browse/ARROW-8536
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-621512162
@paddyhoran any opinion on this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kyprifog commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
kyprifog commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-661880554
What was the resolution here? Using most recent version of arrow-flight crate I get the same error as before.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-622177076
So `rust/arrow-flight/src/arrow.flight.protocol.rs` is generated from `format/Flight.proto`? There is precedent for adding generated files to `rat_excluded_files.txt`, so I think that's fine, though IMO it would be a really good idea to prepend a comment to the generated file that points back to the source and indicates that it should not be edited by hand.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-622927093
@andygrove @nealrichardson I've added the warning. I didn't have a good experience when trying to modify multiple files in Rust (when I was attempting creating a `build.rs` script for the flatbuffer files), but it seems that things are fine with a single file. I was getting sync issues, where my operations weren't being performed by the OS in order.
I see that there's https://github.com/danburkert/prost/issues/308 which was recently opened. I'll remove the hardcoded name in future if the function that compiles protos adds the warning automatically.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-618942929
> @nevi-me This is looking good, but the generated source file needs the ASF header. CI is failing with ` apache-rat license violation: rust/arrow-flight/src/arrow.flight.protocol.rs`.
I've instead added an exception, I hope that's fine
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kyprifog edited a comment on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
kyprifog edited a comment on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-661894115
Sorry I wasn't being clear, I was trying to create a fork of the ballista example by including the ballista crate (v 0.2.5) which was throwing this error on building arrow-flight 0.17.0, but looking at the existing rust example I can see how that won't work.
Edit: I can see ballista crate is up to 0.3.0-SNAPSHOT which built succesfully and I can see includes 1.0.0-SNAPSHOT arrow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kyprifog commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
kyprifog commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-661894115
Sorry I wasn't being clear, I was trying to create a fork of the ballista example by including the ballista crate (v 0.2.5) which was throwing this error on building arrow-flight 0.17.0, but looking at the existing rust example I can see how that won't work now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-618605512
@nevi-me This is looking good, but the generated source file needs the ASF header. CI is failing with ` apache-rat license violation: rust/arrow-flight/src/arrow.flight.protocol.rs`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-622254639
I'll address this later today
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-622174765
@nealrichardson Maybe you have an opinion on this? This is the issue I mentioned on the sync call.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-619202633
Let's see what others say on this. Personally, I think it would be better for build.rs to automatically prepend the ASF license header because there is the risk of someone making manual changes to the file as part of a PR.
I know it is extra work, but it seems safer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-661882919
@kyprifog When you say "most recent version" are you referring to the most recent version published on crates.io (0.17.0) or the latest in the repo (1.0.0-SNAPSHOT)?
The fix will be in the 1.0.0 release which should be published pretty soon (there is currently a vote happening on a release candidate).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kyprifog edited a comment on pull request #7018: ARROW-8536: [Rust] [Flight] Check in proto file, conditional build if file exists
Posted by GitBox <gi...@apache.org>.
kyprifog edited a comment on pull request #7018:
URL: https://github.com/apache/arrow/pull/7018#issuecomment-661894115
Sorry I wasn't being clear, I was trying to create a fork of the ballista example by including the ballista crate (v 0.2.5) which was throwing this error on building arrow-flight 0.17.0, but looking at the existing rust example I can see how that won't work.
Edit: I can see ballista crate is up to 0.3.0-SNAPSHOT which built succesfully, likely includes these new changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org