-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
[fastlane_core] fix non-UTF-8 char issues when analysing ipa #19697
[fastlane_core] fix non-UTF-8 char issues when analysing ipa #19697
Conversation
I signed the CLA now so it should be fine. Could you restart the check? |
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.
Thank you for making this fix!
I did add a commit that should fix an error with an embedded up and also added some tests. Would you be able to review and test this whenever you get a chance? 😊
@@ -73,13 +73,20 @@ def self.fetch_info_plist_with_rubyzip(path) | |||
end | |||
|
|||
def self.fetch_info_plist_with_unzip(path) | |||
list, error, = Open3.capture3("unzip", "-Z", "-1", path) | |||
entry, error, = Open3.capture3("unzip", "-Z", "-1", path, "*/Payload/*.app/Info.plist") |
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.
There is a slight problem with using matching with unzip
😔 Matching on *.app/Info.plist
does not handle the case with multiple apps in a .app
. Like in the case of this WatchKit app 👇
ContainsWatchApp/Payload/Sample.app/Watch/Sample WatchKit App.app/Info.plist
ContainsWatchApp/Payload/Sample.app/Info.plist
I also tried to change it to */Payload/*.app/Info.plist
thinking that this might get the exact match of the main app Info.plist but *.app
seems to behave like **/*.app
😬
HOWEVER... I have a commit that think would fix that that I added 😊 If you would be able to review and test this for me that would be 🔥
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.
Hi Josh! Thank you for fixing this potential bug. I did a simple test on my side but the new code fails, will look into this later and come back to you!
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.
For some reason, */Payload/*.app/Info.plist
can't cover my case although theoretically it should also capture Payload/xxx.app/Info.plist
.
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.
The latest code works for me now, so I assume we're clear for merge?
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.
Thank you so much for finding and debugging this! Excited to get this out 🥳
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.
Congratulations! 🎉 This was released as part of fastlane 2.200.0 🚀
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validMotivation and Context
Fixed an issue (#19692) where
ipa_file_analyser.rb
can't handle non-UTF-8 chars inside file names if the IPA file exceeds 4G.Description
Instead of getting the full file list and then looking for the app's
Info.plist
file, I change the code to look for theInfo.plist
file directly in the zip file.Tested with my own app (around 6.6G) which has files with
Ö
in the file name.Testing Steps