Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support iOS #23

Merged
merged 6 commits into from
Jun 8, 2023
Merged

feat: Support iOS #23

merged 6 commits into from
Jun 8, 2023

Conversation

eseidel
Copy link
Contributor

@eseidel eseidel commented Jun 8, 2023

Adds support for iOS to the updater library.

Refactored the android-specific code into an android module.

The tests still mostly use the android code, rather than testing iOS paths too, but that can be fixed in a follow-up.

I had to change how "latest patch number" works to understand when patch files disappear from disk (which I thought was happening on iOS initially).

I also had to make status.json not store absolute paths to patch files (since the absolute path changes per-xcode run on iOS).

@eseidel eseidel enabled auto-merge (squash) June 8, 2023 21:24
Comment on lines +208 to +211
if self.is_known_bad_patch(&PatchInfo {
path: String::new(),
number: slot.patch_number,
}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a fixme here to update is_known_bad_patch to only require a patch number

inflate(&download_path, base_r, &output_path)
}

#[cfg(not(any(target_os = "android", test)))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on making this explicit that we're targeting iOS (as opposed to "not Android"). Not sure how doable that is since we're also making sure this isn't a test cfg

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case iOS should use this function, just doesn't yet. That's why iOS patch files are MB instead of bytes.

@eseidel eseidel merged commit 50f0b02 into main Jun 8, 2023
@eseidel eseidel deleted the ios_demo branch June 8, 2023 21:34
@@ -154,7 +154,10 @@ impl UpdaterState {
return None;
}
let slot = &self.slots[index];
Some(slot.to_patch_info())
Some(PatchInfo {
path: self.patch_path(index).to_str().unwrap().to_owned(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to unwrap here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Probably isn't. Looking.

fn validate_slot(&self, slot: &Slot) -> bool {
// Check if the patch is known bad.
if self.is_known_bad_patch(&slot.to_patch_info()) {
if self.is_known_bad_patch(&PatchInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the signature to only accept a patch number here so we don’t need to pass an empty path?

.slots
.iter()
.position(|s| s.patch_number == slot.patch_number);
let patch_path = self.patch_path(index.unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about unwrapping and whether it’s safe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants