-
-
Notifications
You must be signed in to change notification settings - Fork 235
feat(preprod): make sure at least one app bundle is present for upload #2795
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
Conversation
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; | ||
} |
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
# Conflicts: # src/utils/build/validation.rs
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.Resolves EME-310