Details

Reviewers

bartek
varun
kamil

Commits

rCOMM6b53895d9758: [native-rust-library] Add backup client dependency

Summary

ENG-5556
Add the backup client to the native_rust library. We activate different features depending on a platform - on iOS we can use the native tls implementation but on Android the native tls reqwest feature pulls in openssl (beause it's linux) and it conflicts with our openssl during linking process. Because of that we replace it with a pure-rust implementation rustls-tls.

Depends on D10261

Test Plan

Test that the app compiles on an physical Android and iOS devices.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Dec 8 2023, 8:28 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptDec 8 2023, 8:29 AM

michal added a child revision: D10264: [native-rust-library] Upload/download user backup.Dec 8 2023, 8:35 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 8 2023, 8:55 AM

Harbormaster failed remote builds in B24906: Diff 34440!

Move the target dependant features to backup_client to check if that fixes the CI issue

Try adding resolver = 2 (but that should already be the default based on the edition...)

Harbormaster failed remote builds in B24938: Diff 34473!Dec 11 2023, 7:01 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 11 2023, 7:22 AM

Harbormaster failed remote builds in B24940: Diff 34475!

Try always setting rustls-tls

Harbormaster completed remote builds in B24941: Diff 34476.Dec 11 2023, 9:10 AM

michal requested review of this revision.Dec 11 2023, 9:10 AM

michal planned changes to this revision.Dec 11 2023, 9:11 AM

Try installing pkg-config on the CI runner

michal planned changes to this revision.Dec 12 2023, 3:02 AM

Harbormaster completed remote builds in B24984: Diff 34522.Dec 12 2023, 3:38 AM

Using @atul suggestion from ENG-5556, I just added pkg-config to buildkite android config. I couldn't figure out a better solution...

Harbormaster completed remote builds in B24986: Diff 34526.Dec 12 2023, 4:21 AM

bartek accepted this revision.Dec 13 2023, 11:45 PM

This revision is now accepted and ready to land.Dec 13 2023, 11:45 PM

varun requested changes to this revision.Dec 14 2023, 10:12 AM

varun added inline comments.

.buildkite/android.yml
7 ↗	(On Diff #34526)	just to confirm, devs don't need to install anything new in their local environments, right?
native/native_rust_library/Cargo.toml
20–41 ↗	(On Diff #34526)	we already have an android feature. do we really need to add logic conditioned on the target os?

This revision now requires changes to proceed.Dec 14 2023, 10:12 AM

michal requested review of this revision.Dec 18 2023, 2:57 AM

michal added inline comments.

.buildkite/android.yml
7 ↗	(On Diff #34526)	Looks like `pkg-config` is included in nix config link, so I don't think so
native/native_rust_library/Cargo.toml
20–41 ↗	(On Diff #34526)	One problem with that is that features are additive and the `default` feature will still be active on android, which was the whole point of adding this code (openssl build issues on android). In general I find it weird that we have an `android` feature. Is there some explicit reason for it? Rust has a builtin way to conditionally run code/ include dependencies based on the platform so I think we should use it.

varun accepted this revision.Dec 18 2023, 10:52 PM

varun added inline comments.

native/native_rust_library/Cargo.toml
20–41 ↗	(On Diff #34526)	One problem with that is that features are additive and the default feature will still be active on android, which was the whole point of adding this code (openssl build issues on android). `default-features = false` solves this, but i understand your point In general I find it weird that we have an android feature. Is there some explicit reason for it? Rust has a builtin way to conditionally run code/ include dependencies based on the platform so I think we should use it. I think the android feature is weird, too. I'm going to remove it and follow your approach instead.