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

Added umbrella header to allow for including the framework in playgrounds #1

Closed
wants to merge 9 commits into from

Conversation

Copy link
Member

@ffried ffried left a comment

Choose a reason for hiding this comment

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

Generally looks good. Only small warning to fix. 🙂


#import "unzip.h"
#import "zip.h"

Copy link
Member

Choose a reason for hiding this comment

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

An #import "crypt.h" is missing here.
Compiling also warns about it:

<module-includes>:1:1: warning: umbrella header for module 'Minizip' does not include header 'crypt.h'
#include "/Zip/Zip/minizip/include/Minizip.h"
^

Copy link
Author

Choose a reason for hiding this comment

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

Hey Florian, I addressed that warning, but in the process came across a few more things worth addressing.
Most notably, I overwrote the project with the results of swift package generate-xcodeproj because the project wasn't building via Xcode due to the removal of the module.modulemap file.
This may endanger the desirability of the original SPM support PR back into the root marmelroy/Zip repo, so you may want to reconsider my PR.
I've made the changes in a series of commits so you may want to cherry-pick whichever bits you like.
¡Thanks for the feedback!

Copy link
Member

Choose a reason for hiding this comment

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

@CalQL8ed-K-OS Yea, overwriting the Xcode project isn't a good idea for non-SPM projects. 🙂
I'll check if I can manually integrate your changes. Maybe I can just tell SPM to ignore the module map.

Message from `swift build` was:
```
<module-includes>:1:1: warning: umbrella header for module 'Minizip' does not include header 'crypt.h'
#import "/Users/CalQL8ed-K-OS/Workbench/open-source/Zip/Zip/minizip/include/Minizip.h"
```
The project was not building due to the removal of the file `module.modulemap` which collected the C code into the `import`-able module *Minizip*.
@ffried
Copy link
Member

ffried commented Mar 27, 2019

@CalQL8ed-K-OS So, I've finally found time to look at your changes again and integrate them in my branch.
Like I mentioned, we can't re-generate the Xcode Project with SPM, since this would break non-SPM projects. But I've nevertheless managed to get rid of (or actually move) the module.modulemap file.

Could you maybe check if this now works with playgrounds?

I'll close this PR since its main feature should be integrated in the branch now.

@ffried ffried closed this Mar 27, 2019
@ghost
Copy link
Author

ghost commented Apr 29, 2019

@CalQL8ed-K-OS So, I've finally found time to look at your changes again and integrate them in my branch.
Like I mentioned, we can't re-generate the Xcode Project with SPM, since this would break non-SPM projects. But I've nevertheless managed to get rid of (or actually move) the module.modulemap file.

Could you maybe check if this now works with playgrounds?

I'll close this PR since its main feature should be integrated in the branch now.

I've created a new PR, #2, which adds a playground and demonstrates that this branch does indeed work with playgrounds now.
FYI This playground was created using John Sundell's Playground CLI Tool, specifically the command was playground -t Zip.playground -d ./Zip.xcodeproj

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.

2 participants