@dcheng, can you PTAL? Assigning directly to you because this needs a review from `build/rust/OWNERS`:
* There are some changes in `build.rs`. The changes look okay (i.e. hermetic), but they require:
* Setting `CARGO_PKG_VERSION_PATCH` environment variable in `cargo_crate.gni`
* Tweaking `gnrt_config.toml` to indicate `build_script_outputs`
* Because of splitting `serde` into `serde_core` + `serde`, we also needed some additional manual tweaks:
* We can now say `allow_unsafe = false` for `serde`
* And we also need to update `components/cronet/android/dependencies.txt`
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Code-Review | +1 |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Auto-Submit | +1 |
+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roll serde: 1.0.219 => 1.0.225 in //third_party/rust.
This CL has been created semi-automatically. The expected review
process and other details can be found at
//tools/crates/create_update_cl.md
Updated crates:
* serde: 1.0.219 => 1.0.225; https://docs.rs/crate/serde/1.0.225
* serde_derive: 1.0.219 => 1.0.225;
https://docs.rs/crate/serde_derive/1.0.225
New crates:
* serde...@1.0.225; https://docs.rs/crate/serde_core/1.0.225
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Is it possible to revert this CL? The new version of `serde` has introduced some complications to our Android pipeline because of this line:
```
include!(concat!(env!("OUT_DIR"), "/private.rs"));
```
`/private.rs` is generated via the build_script. We currently support the usual case of `build_scripts` where it generates a bunch of `--cfg` and `--features` that are passed to the build system afterwards. However, passing a rust file that is generated from build scripts is something that we have never encountered before and has broken our pipeline. It's also important to mention that this version has not propagated to AOSP yet, so the rust folk in AOSP might not have faced this problem yet too. We expected that this will happen at some point (go/rust-assumptions-for-gn2bp) but I guess now is the time.
So if possible, would it be okay to revert this in the meantime while I work on adding support for this just to unblock our imports?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Is it possible to revert this CL? The new version of `serde` has introduced some complications to our Android pipeline because of this line:
```
include!(concat!(env!("OUT_DIR"), "/private.rs"));
````/private.rs` is generated via the build_script. We currently support the usual case of `build_scripts` where it generates a bunch of `--cfg` and `--features` that are passed to the build system afterwards. However, passing a rust file that is generated from build scripts is something that we have never encountered before and has broken our pipeline. It's also important to mention that this version has not propagated to AOSP yet, so the rust folk in AOSP might not have faced this problem yet too. We expected that this will happen at some point (go/rust-assumptions-for-gn2bp) but I guess now is the time.
So if possible, would it be okay to revert this in the meantime while I work on adding support for this just to unblock our imports?
Hi Mohannad, how large a change do you think it will be to accommodate this? i.e. if we revert, for how long are you asking us to hold the roll?
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Is it possible to revert this CL? The new version of `serde` has introduced some complications to our Android pipeline because of this line:
```
include!(concat!(env!("OUT_DIR"), "/private.rs"));
````/private.rs` is generated via the build_script. We currently support the usual case of `build_scripts` where it generates a bunch of `--cfg` and `--features` that are passed to the build system afterwards. However, passing a rust file that is generated from build scripts is something that we have never encountered before and has broken our pipeline. It's also important to mention that this version has not propagated to AOSP yet, so the rust folk in AOSP might not have faced this problem yet too. We expected that this will happen at some point (go/rust-assumptions-for-gn2bp) but I guess now is the time.
So if possible, would it be okay to revert this in the meantime while I work on adding support for this just to unblock our imports?
We have another weekly crate update in progress, so to avoid mid-air collisions I'll wait until those CLs land (in a day or two) and then I can try reverting.
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Łukasz AnforowiczIs it possible to revert this CL? The new version of `serde` has introduced some complications to our Android pipeline because of this line:
```
include!(concat!(env!("OUT_DIR"), "/private.rs"));
````/private.rs` is generated via the build_script. We currently support the usual case of `build_scripts` where it generates a bunch of `--cfg` and `--features` that are passed to the build system afterwards. However, passing a rust file that is generated from build scripts is something that we have never encountered before and has broken our pipeline. It's also important to mention that this version has not propagated to AOSP yet, so the rust folk in AOSP might not have faced this problem yet too. We expected that this will happen at some point (go/rust-assumptions-for-gn2bp) but I guess now is the time.
So if possible, would it be okay to revert this in the meantime while I work on adding support for this just to unblock our imports?
We have another weekly crate update in progress, so to avoid mid-air collisions I'll wait until those CLs land (in a day or two) and then I can try reverting.
FWIW I was able to revert locally, but I haven't uploaded yet, because I am waiting for https://crrev.com/c/6977866 to land and get out of the way first (it hasn't landed yet because the tree seems to be having some trouble today and got closed multiple times because of MSAN issues).
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Łukasz AnforowiczIs it possible to revert this CL? The new version of `serde` has introduced some complications to our Android pipeline because of this line:
```
include!(concat!(env!("OUT_DIR"), "/private.rs"));
````/private.rs` is generated via the build_script. We currently support the usual case of `build_scripts` where it generates a bunch of `--cfg` and `--features` that are passed to the build system afterwards. However, passing a rust file that is generated from build scripts is something that we have never encountered before and has broken our pipeline. It's also important to mention that this version has not propagated to AOSP yet, so the rust folk in AOSP might not have faced this problem yet too. We expected that this will happen at some point (go/rust-assumptions-for-gn2bp) but I guess now is the time.
So if possible, would it be okay to revert this in the meantime while I work on adding support for this just to unblock our imports?
Łukasz AnforowiczWe have another weekly crate update in progress, so to avoid mid-air collisions I'll wait until those CLs land (in a day or two) and then I can try reverting.
FWIW I was able to revert locally, but I haven't uploaded yet, because I am waiting for https://crrev.com/c/6977866 to land and get out of the way first (it hasn't landed yet because the tree seems to be having some trouble today and got closed multiple times because of MSAN issues).
I expect this to take around a week. I've already managed to get our pipeline to be green with local changes. I'd still need to clean-up my local changes, upload them for review and confirm that everything goes as expected. So a week is a maximum that I'll ask of you. If possible
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Łukasz AnforowiczIs it possible to revert this CL? The new version of `serde` has introduced some complications to our Android pipeline because of this line:
```
include!(concat!(env!("OUT_DIR"), "/private.rs"));
````/private.rs` is generated via the build_script. We currently support the usual case of `build_scripts` where it generates a bunch of `--cfg` and `--features` that are passed to the build system afterwards. However, passing a rust file that is generated from build scripts is something that we have never encountered before and has broken our pipeline. It's also important to mention that this version has not propagated to AOSP yet, so the rust folk in AOSP might not have faced this problem yet too. We expected that this will happen at some point (go/rust-assumptions-for-gn2bp) but I guess now is the time.
So if possible, would it be okay to revert this in the meantime while I work on adding support for this just to unblock our imports?
Łukasz AnforowiczWe have another weekly crate update in progress, so to avoid mid-air collisions I'll wait until those CLs land (in a day or two) and then I can try reverting.
Mohannad FarragFWIW I was able to revert locally, but I haven't uploaded yet, because I am waiting for https://crrev.com/c/6977866 to land and get out of the way first (it hasn't landed yet because the tree seems to be having some trouble today and got closed multiple times because of MSAN issues).
I expect this to take around a week. I've already managed to get our pipeline to be green with local changes. I'd still need to clean-up my local changes, upload them for review and confirm that everything goes as expected. So a week is a maximum that I'll ask of you. If possible
If it's too much of a hassle, then we can just fix-forward.
Mohannad Farrag+@aymanm for `//components/cronet/android/dependencies.txt` - PTAL?
Łukasz AnforowiczIs it possible to revert this CL? The new version of `serde` has introduced some complications to our Android pipeline because of this line:
```
include!(concat!(env!("OUT_DIR"), "/private.rs"));
````/private.rs` is generated via the build_script. We currently support the usual case of `build_scripts` where it generates a bunch of `--cfg` and `--features` that are passed to the build system afterwards. However, passing a rust file that is generated from build scripts is something that we have never encountered before and has broken our pipeline. It's also important to mention that this version has not propagated to AOSP yet, so the rust folk in AOSP might not have faced this problem yet too. We expected that this will happen at some point (go/rust-assumptions-for-gn2bp) but I guess now is the time.
So if possible, would it be okay to revert this in the meantime while I work on adding support for this just to unblock our imports?
Łukasz AnforowiczWe have another weekly crate update in progress, so to avoid mid-air collisions I'll wait until those CLs land (in a day or two) and then I can try reverting.
Mohannad FarragFWIW I was able to revert locally, but I haven't uploaded yet, because I am waiting for https://crrev.com/c/6977866 to land and get out of the way first (it hasn't landed yet because the tree seems to be having some trouble today and got closed multiple times because of MSAN issues).
Mohannad FarragI expect this to take around a week. I've already managed to get our pipeline to be green with local changes. I'd still need to clean-up my local changes, upload them for review and confirm that everything goes as expected. So a week is a maximum that I'll ask of you. If possible
If it's too much of a hassle, then we can just fix-forward.
Yeah, sorry, but the revert indeed does seem like quite a bit of trouble. I tried to actually run `gn gen` after the revert and it failed - this seems to be caused by the following transitive dependency on the rolled package (i.e. the revert is difficult and would necessitate cascading/follow-up reverts/changes [at least also reverting the roll of `serde_json`):
```
$ tools/crates/run_gnrt.py vendor
Finished `release` profile [optimized] target(s) in 0.19s
Running `out/gnrt/target/release/gnrt vendor`
Vendoring crates from third_party/rust/chromium_crates_io
Error: running cargo metadata
Caused by:
0: `cargo metadata` execution failed
1: `cargo metadata` exited with an error: error: failed to select a version for `serde`.
... required by package `serde_json v1.0.145`
... which satisfies dependency `serde_json = "^1.0.138"` (locked to 1.0.145) of package `llguidance v1.2.0`
... which satisfies dependency `llguidance = "^1.0.0"` (locked to 1.2.0) of package `chromium v0.1.0 (/usr/local/google/home/lukasza/src/chromium2/src/third_party/rust/chromium_crates_io)`
versions that meet the requirements `^1.0.220` are: 1.0.226, 1.0.225, 1.0.224, 1.0.223, 1.0.222, 1.0.221, 1.0.220
all possible versions conflict with previously selected packages.
previously selected package `serde v1.0.219`
... which satisfies dependency `serde = "^1.0.219"` (locked to 1.0.219) of package `chromium v0.1.0 (/usr/local/google/home/lukasza/src/chromium2/src/third_party/rust/chromium_crates_io)`
failed to select a version for `serde` which could resolve this conflict
```
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |