Skip to content

Conversation

trevor-e
Copy link
Member

@trevor-e trevor-e commented Sep 23, 2025

Another edge case we see is users accidentally uploading an empty xcarchive to us. This will flag an error log if we detect no .app bundles were detected.

  ERROR   2025-09-23 12:41:50.487294 -04:00 Invalid XCArchive: No .app bundles found in the Products/ directory
error: File is not a recognized supported build format (APK, AAB, XCArchive, or IPA): /Users/telkins/Downloads/<redacted>.xcarchive

Resolves EME-310

@trevor-e trevor-e requested review from szokeasaurusrex and a team as code owners September 23, 2025 16:44
Copy link

linear bot commented Sep 23, 2025

Comment on lines 92 to 96
let app_paths: Vec<_> = paths.flatten().filter(|path| path.is_dir()).collect();
if app_paths.is_empty() {
error!("Invalid XCArchive: No .app bundles found in the Products/ directory");
return false;
}
Copy link
Member

@szokeasaurusrex szokeasaurusrex Sep 24, 2025

Choose a reason for hiding this comment

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

l: Here's a little tip to make this code more efficient:

Suggested change
let app_paths: Vec<_> = paths.flatten().filter(|path| path.is_dir()).collect();
if app_paths.is_empty() {
error!("Invalid XCArchive: No .app bundles found in the Products/ directory");
return false;
}
let mut app_paths = paths.flatten().filter(|path| path.is_dir()).peekable();
if app_paths.peek().is_none() {
error!("Invalid XCArchive: No .app bundles found in the Products/ directory");
return false;
}

The difference is that, in your proposal, you are eagerly computing the filtered app_paths, and you are allocating a whole vector to store all of them. If paths is large, then this is potentially compute-inefficient and memory-inefficient.

With my suggestion, you avoid eagerly computing the entire app_paths. Instead, we only peek the first item, which means internally, the iterator only needs to compute and store the first value. Because iterators in Rust are lazy, we only compute the values as we iterate over them, saving ourselves a memory allocation, and potentially saving us from having to iterate all of the paths, in the case where we detect an invalid XCArchive and exit early.

I suspect the actual difference in performance is quite small here, but I want to highlight this, as collect is an expensive operation (it iterates the entire iterator), and in general, we should defer calling collect until as late as possible or avoid calling it entirely, as we do here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think eagerly collecting makes more sense in this case since paths is going to be small, <= 5 for basically 99.9% of apps we see. Working directly with the iterator like that makes the code too clever and less readable IMO, e.g. now app_paths has to be mutable.

Copy link
Member

Choose a reason for hiding this comment

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

That's also fair, admittedly I did not know how many paths we typically expect. If it's smaller than the readability improvement from collecting is likely worth it

Base automatically changed from telkins/check-info-plist to master September 24, 2025 18:40
@trevor-e trevor-e requested a review from a team as a code owner September 24, 2025 18:40
# Conflicts:
#	src/utils/build/validation.rs
@trevor-e trevor-e merged commit 1b34a33 into master Sep 24, 2025
25 checks passed
@trevor-e trevor-e deleted the telkins/empty-products branch September 24, 2025 19:14
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