Page MenuHomePhabricator
Differential D10263

[native-rust-library] Add backup client dependency
ClosedPublic
Actions

Authored by michal on Dec 8 2023, 8:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 2:08 AM
Unknown Object (File)
Tue, Nov 12, 4:37 PM
Unknown Object (File)
Thu, Nov 7, 5:50 PM
Unknown Object (File)
Thu, Nov 7, 12:25 AM
Unknown Object (File)
Oct 18 2024, 2:43 AM
Unknown Object (File)
Oct 18 2024, 2:43 AM
Unknown Object (File)
Oct 18 2024, 2:43 AM
Unknown Object (File)
Oct 18 2024, 2:43 AM
View All Files
Subscribers
ashoat
atul
tomek

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
No Lint Coverage
Unit
No Test Coverage

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!
michal updated this revision to Diff 34473.Dec 11 2023, 6:35 AM

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

michal updated this revision to Diff 34475.Dec 11 2023, 6:55 AM

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!
michal updated this revision to Diff 34476.Dec 11 2023, 8:38 AM

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
michal updated this revision to Diff 34522.Dec 12 2023, 3:01 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
michal updated this revision to Diff 34526.Dec 12 2023, 3:52 AM
michal added a subscriber: atul.

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.

This revision is now accepted and ready to land.Dec 18 2023, 10:52 PM
varun mentioned this in D10394: [native] remove android feature.Dec 18 2023, 10:58 PM
Closed by commit rCOMM6b53895d9758: [native-rust-library] Add backup client dependency (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyJan 3 2024, 7:04 AM
This revision was automatically updated to reflect the committed changes.
Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM6b53895d9758: [native-rust-library] Add backup client dependency.
varun mentioned this in rCOMMd822ce2d1d08: [native] remove android feature.Jan 5 2024, 11:01 PM

Revision Contents
Changeset List

PathSize
.buildkite/
2 lines
native/
native_rust_library/
13 lines
shared/
backup_client/

Diff 34522

View Options

.buildkite/android.yml

Loading...
View Options

native/native_rust_library/Cargo.lock

Loading...
View Options

native/native_rust_library/Cargo.toml

Loading...
View Options

shared/backup_client/Cargo.lock

Loading...