[rust] Split the `rust_logger` crate into smaller modules. [chromium/src : main]
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
@dcheng, can you PTAL as one of `//base/OWNERS` (and as a Chrome Rust co-conspirator)? This CL mostly just moves stuff / shouldn't change the behavior. See the CL description for more details.
/cc @dloehr in case this helps with the discussions about what shape Rust stuff can/should have
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
2 comments
LGTM; if we need to do more of these in the future, might be nice to break out each module into a separate CL for reviewability, though I know that's a bit annoying depending on the Gerrit workflow you use...
log::Level::Error => print_rust_log::LogSeverity::Error,Because things like this aren't quite identical and I need to stare harder at the change :)
Related details
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
LGTM; if we need to do more of these in the future, might be nice to break out each module into a separate CL for reviewability, though I know that's a bit annoying depending on the Gerrit workflow you use...
Ack. I guess I could have first had a CL for introducing modules **within** the original `logger.rs` and then moving things to a new directory in a separate CL.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Łukasz Anforowicz
| Auto-Submit | +1 |
1 comment
@aymanm, can you PTAL at `//components/cronet/Android/dependencies.txt`?
Related details
- Mohannad Farrag
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag added 1 comment![]( "Open in Gerrit")
@aymanm, can you PTAL at `//components/cronet/Android/dependencies.txt`?
I've started our gn2bp bot. This looks like a scary CL. I just want to see if it will choke on it or not. I'm unlikely to block you if it breaks, just want to see what is the result.
Related details
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 1 comment![]( "Open in Gerrit")
Mohannad Farrag@aymanm, can you PTAL at `//components/cronet/Android/dependencies.txt`?
I've started our gn2bp bot. This looks like a scary CL. I just want to see if it will choke on it or not. I'm unlikely to block you if it breaks, just want to see what is the result.
What makes the CL scary / risky? FWIW the CL doesn't change which `gni` templates (e.g. `rust_static_library` or `rust_bindgen`) are used - it only moves them around a bit. OTOH I see that https://googleplex-android-review.git.corp.google.com/c/platform/external/cronet/+/39258778 did indeed fail some checks - e.g. https://android-build.corp.google.com/build_explorer/artifact_viewer/P120382460/cf_x86_64_wear-trunk_staging-userdebug/logs/build_error.log?attemptId=latest says:
```
error: unexpected argument 'external/cronet/tot/base/logging/rust_logger/print_rust_log.rs' found
Usage: cxxbridge <input>.rs Emit .cc file for bridge to stdout
cxxbridge <input>.rs --header Emit .h file for bridge to stdout
cxxbridge --header Emit "rust/cxx.h" header to stdout
```
It seems that gn2bp doesn't support multiple files being listed in `rust_static_library.cxx_bindings`.
So, what are the next steps here?
Related details
- Mohannad Farrag
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag added 1 comment![]( "Open in Gerrit")
Mohannad Farrag@aymanm, can you PTAL at `//components/cronet/Android/dependencies.txt`?
Łukasz AnforowiczI've started our gn2bp bot. This looks like a scary CL. I just want to see if it will choke on it or not. I'm unlikely to block you if it breaks, just want to see what is the result.
What makes the CL scary / risky? FWIW the CL doesn't change which `gni` templates (e.g. `rust_static_library` or `rust_bindgen`) are used - it only moves them around a bit. OTOH I see that https://googleplex-android-review.git.corp.google.com/c/platform/external/cronet/+/39258778 did indeed fail some checks - e.g. https://android-build.corp.google.com/build_explorer/artifact_viewer/P120382460/cf_x86_64_wear-trunk_staging-userdebug/logs/build_error.log?attemptId=latest says:
```
error: unexpected argument 'external/cronet/tot/base/logging/rust_logger/print_rust_log.rs' foundUsage: cxxbridge <input>.rs Emit .cc file for bridge to stdout
cxxbridge <input>.rs --header Emit .h file for bridge to stdout
cxxbridge --header Emit "rust/cxx.h" header to stdout
```It seems that gn2bp doesn't support multiple files being listed in `rust_static_library.cxx_bindings`.
So, what are the next steps here?
Yeah. This is a bug in GN2BP that we should fix. Let me ask, how pressing is this CL so I can evaluate the risk of fixing forward or just hold until we fix GN2BP to support multiple files (should be relatively straightforward).
Related details
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag added 1 comment![]( "Open in Gerrit")
Mohannad Farrag@aymanm, can you PTAL at `//components/cronet/Android/dependencies.txt`?
Łukasz AnforowiczI've started our gn2bp bot. This looks like a scary CL. I just want to see if it will choke on it or not. I'm unlikely to block you if it breaks, just want to see what is the result.
Mohannad FarragWhat makes the CL scary / risky? FWIW the CL doesn't change which `gni` templates (e.g. `rust_static_library` or `rust_bindgen`) are used - it only moves them around a bit. OTOH I see that https://googleplex-android-review.git.corp.google.com/c/platform/external/cronet/+/39258778 did indeed fail some checks - e.g. https://android-build.corp.google.com/build_explorer/artifact_viewer/P120382460/cf_x86_64_wear-trunk_staging-userdebug/logs/build_error.log?attemptId=latest says:
```
error: unexpected argument 'external/cronet/tot/base/logging/rust_logger/print_rust_log.rs' foundUsage: cxxbridge <input>.rs Emit .cc file for bridge to stdout
cxxbridge <input>.rs --header Emit .h file for bridge to stdout
cxxbridge --header Emit "rust/cxx.h" header to stdout
```It seems that gn2bp doesn't support multiple files being listed in `rust_static_library.cxx_bindings`.
So, what are the next steps here?
Yeah. This is a bug in GN2BP that we should fix. Let me ask, how pressing is this CL so I can evaluate the risk of fixing forward or just hold until we fix GN2BP to support multiple files (should be relatively straightforward).
I've created https://crrev.com/c/7726224 which potentially should fix this issue. I'll run presubmit on it. If it passes then I'll merge it with your CL and both can be submitted.
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Łukasz Anforowicz
| Auto-Submit | +1 |
1 comment
Thanks @aymanm for updating `components/cronet/gn2bp/gen_android_bp.py` for this CL. Can you PTAL and add the CR+1 stamp if everything looks good to you?
Related details
- Mohannad Farrag
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |