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

First add for minizip #1

Merged
merged 5 commits into from
Dec 15, 2023
Merged

First add for minizip #1

merged 5 commits into from
Dec 15, 2023

Conversation

ryanskeith
Copy link

Minizip 4.0.3

Destination channel:
main

Links

Explanation of changes:

  • added gtest to avoid code from downloading from source and building
  • added gtest to ignore_run_exports as it is a test dependency and not a run dependency

recipe/meta.yaml Outdated Show resolved Hide resolved
Copy link

@danpetry danpetry left a comment

Choose a reason for hiding this comment

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

A few comments for where I couldn't put comments in the code:

  • probably better to explicitly state the build optionsthat are being used, for documentation purposes. I mean the ones that are default on and are already being used because the dependencies are there in the host environment
  • you could build tests and unit tests for windows too
  • do you know why the static library is included in the package for windows but not unix?
  • also why CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is on?

recipe/meta.yaml Show resolved Hide resolved
@ryanskeith
Copy link
Author

A few comments for where I couldn't put comments in the code:

  • probably better to explicitly state the build optionsthat are being used, for documentation purposes. I mean the ones that are default on and are already being used because the dependencies are there in the host environment
  • you could build tests and unit tests for windows too
  • do you know why the static library is included in the package for windows but not unix?
  • also why CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is on?
  • That's a good idea on being explicit. I will flesh those out.

  • I will try adding tests for windows.

  • I do not know why static libs are included in the package for windows and not unix.

  • I am not sure why Windows Export all symbols was chosen as well.

Copy link

@skupr-anaconda skupr-anaconda left a comment

Choose a reason for hiding this comment

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

LGTM but you can use ninja-base instead of ninja

Copy link

@danpetry danpetry left a comment

Choose a reason for hiding this comment

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

I do not know why static libs are included in the package for windows and not unix.

I am not sure why Windows Export all symbols was chosen as well.

ok, I don't think it's a big deal, the package isn't used anywhere at the moment

@ryanskeith
Copy link
Author

I found out why windows exports are the way they are found in this issue that is yet to be resolved: zlib-ng/minizip-ng#475

@ryanskeith ryanskeith merged commit a43e329 into main Dec 15, 2023
8 checks passed
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.

4 participants