I wonder if this logic might be a bit too complex for what we need—is there any reason why we can't simply check for Info.plist at the root of the zip, then check for Info.plist inside the first nested folder? We could probably just throw an error if it doesn't exist in either of those locations.

I'm wondering how often the structure of the zip would actually be more complicated than that. For applications built with standard tooling you would probably only ever find the .app at 0 or 1 levels deep.

Hi @lfroms, it's a good question, let me try answer it:

"First" (quoted because zip archives have no well-defined ordering) may not necessarily be the correct app folder. For example, if the archive contains a metadata folder (e.g. screenshots, or a dot-folder) then the first folder may unluckily be the wrong folder. Regardless, finding the first folder involves scanning the contents of the zip archive, at this stage the decision is whether you blindly accept the first folder or try to find the right folder (I decided that was one containing an Info.plist).

Once you're already iterating the zip archive looking for the right first folder, the logic isn't much less complicated than doing it for nested folders, and I thought it might be prudent to avoid the reoccurence of the "Tophat doesn't work with app bundles nested N+1 levels deep" issues.

Hopefully this gives you some insight into my journey to get to this point.

I'm not super convinced that we would often encounter a structure other than at the 0th or 1st level. Right-clicking and compressing in Finder or Xcode's default output yields one level of nesting, or zipping the contents yields 0 levels of nesting. I think that should be enough to cover 99% of use cases.

Could we update the logic to match that?

So to confirm my understanding, you're suggesting that if there is no Info.plist at the root of the archive, then look only in the first directory in the archive for an Info.plist, regardless of what that directory might be or whether there are other directories?

Yes. And maybe what we can do just to be safe is to iterate over all top-level subdirectories but I wouldn't go further than that. I think it's best to keep things as simple as possible here given that I think it's unlikely that we'll often encounter a more complex structure.

Also, I don't think we necessarily need the temporary directory. We can probably just return the deeper URL from the function.

And I'd make sure that if nothing is found in these two cases to just throw an error. One thing that I do wonder is how this behaves when handling APKs or IPAs, would this logic handle those gracefully? Perhaps we should actually be checking for entries with extension .app, .ipa or .apk instead...

If extractArtifact throws in the case of Info.plist not being found, wouldn't that cause a regression for zipped IPAs and APKs?

Since unpack has the detection logic, maybe a better option is to leave extractArtfifact simply extracting things, and extend the .applicationBundle case in unpack:

• Look in the artifactURL app bundle and see if there is an Info.plist direct child
• If there is, then there is nothing to do and continue with artifactURL
• If there isn't (e.g. MyApp.app.zip extracts to MyApp.app/MyApp.app/Info.plist), then scan the first level of directories for the first one that contains Info.plist, and continue with the amended artifactURL
• Otherwise throw an error regarding a corrupted app bundle

This would localise the changes to only handling app bundles.

One caveat is that this wouldn't change anything if the extracted structure was something like build/MyApp.zip because build doesn't have a .app extension causing the guard in unpack to throw, which was implicitly solved by doing the scanning in the zip archive. But I don't think that was the original intention anyway.

We would want to handle the different file formats separately, right? Each one should enter a different code path with different logic for detecting nesting. We would only be checking for Info.plist for application bundles, for example.

@lfroms I've pushed a commit that applies your suggestions from this thread, summarised they are:

• If the extracted destination is a directory, only scan the first level of the directory
• If Info.plist exists as a direct child, then return the original destination URL
• If a .app exists and it has a direct Info.plist child, return the URL to it, letting unpack do the rest of the work
• If a .ipa or .apk exists, then return their URL as the destination, letting unpack do the rest of the work
• Other known formats (e.g. .zip) and unknown formats are ignored during the scan

Because extracting an app store package also used extractArtifact, I opted to add a parameter to decide whether to examine the extraction further, which only the .zip case uses. If you'd prefer that this is a separate method, rather than a flag, I'm happy to change that.

In case you're wondering: This code opts to do this on the filesystem, rather than within the zip archive, because zip archives have no explicit concept of hierarchy, so scanning "the first level" would require filtering the archive contents based on some invented predicate. I figured that using filesystem tools already in Foundation was better than writing more of these by hand.

I opted to leave the unpacking logic in unpack for the formats, and have the extracting logic only go to the point where it was reasonably sure the result was unpackable. This seemed cleaner than involving directory scanning logic in unpack.

I tested a variation of each of these cases (.ipa, .ipa.zip, .apk, .apk.zip, .app.zip; all with and without nesting) via a local HTTP recipe.

Hi @jonathanj, sorry for the delay in getting back to this.

I think the logic and assumptions you're proposing in your latest revision are sound. Just one note: I'm hoping we can avoid a case where an artifact with name MyApp.app.zip which also has a subdirectory does not extract into a parent directory called MyApp.app, even though the code will return MyApp.app/MyApp.app. It still works, but it's an invalid bundle that I'm hoping to avoid creating.

With that in mind, I'd like to propose an alternative implementation that does not require an additional switch on file formats by instead combining with the initial check in unpack(artifactURL:) and locating a suitable enclosed artifact there:

See branch: main...nested-zip-example

If this looks good and works for you, feel free to incorporate it into this branch and then I'll go ahead and merge this. Thanks!