Skip to content

Use system zlib instead of bundled.#137

Merged
Lgt2x merged 1 commit intoDescentDevelopers:mainfrom
Nakhr11n:unzip
Apr 24, 2024
Merged

Use system zlib instead of bundled.#137
Lgt2x merged 1 commit intoDescentDevelopers:mainfrom
Nakhr11n:unzip

Conversation

@Nakhr11n
Copy link
Contributor

It's unnecessary to keep zlib in the project files.
Better to link against system zlib, which is probably more up to date.

If we really want to link statically, I would suggest using CMake ExternalProject to do so.

@Lgt2x
Copy link
Member

Lgt2x commented Apr 21, 2024

CI scripts and build instructions will need updated to install zlib

@Nakhr11n Nakhr11n marked this pull request as draft April 21, 2024 17:02
@Nakhr11n Nakhr11n force-pushed the unzip branch 2 times, most recently from 9dacefe to 2bbf7ec Compare April 21, 2024 18:49
@Nakhr11n Nakhr11n marked this pull request as ready for review April 21, 2024 18:50
@Nakhr11n
Copy link
Contributor Author

CI and build instructions updated.

@winterheart
Copy link
Collaborator

Better to use vcpkg to install required dependencies. It has native cmake integration via toolchain files and can build zlib statically on request.

@Nakhr11n Nakhr11n marked this pull request as draft April 21, 2024 19:00
@Nakhr11n
Copy link
Contributor Author

Nakhr11n commented Apr 21, 2024

Forgot about Mac and Windows.
They work now.

@Nakhr11n Nakhr11n marked this pull request as ready for review April 21, 2024 19:20
@Nakhr11n Nakhr11n marked this pull request as draft April 21, 2024 19:30
@Nakhr11n Nakhr11n marked this pull request as ready for review April 21, 2024 19:45
@Nakhr11n Nakhr11n force-pushed the unzip branch 2 times, most recently from 5f1bc5c to 6d7a222 Compare April 21, 2024 20:00
@Nakhr11n Nakhr11n marked this pull request as draft April 21, 2024 20:22
@Nakhr11n Nakhr11n force-pushed the unzip branch 3 times, most recently from dd42a19 to 28dc999 Compare April 22, 2024 09:22
@Nakhr11n Nakhr11n marked this pull request as ready for review April 22, 2024 09:23
@Nakhr11n
Copy link
Contributor Author

Now the vcpkg issues are resolved. In github actions the vcpkg path is hardcoded since the VCPKG_ROOT envvar isn't set. Otherwise it uses VCPKG_ROOT.

@Lgt2x
Copy link
Member

Lgt2x commented Apr 23, 2024

I'm being nit-picky here but could you please update README build instructions for Windows users to include VCPKG please ? @Arcnor can confirm but after that I think we're ready to merge :)

@Arcnor
Copy link
Contributor

Arcnor commented Apr 23, 2024

Yeah, it will be great, because this PR introduced extra steps to build the project.

You can just mention the need to define VCPKG_ROOT before calling the CMake commands.

@Nakhr11n
Copy link
Contributor Author

Now there is no more extra github actions step, vcpkg has been included in the readme.

@Arcnor
Copy link
Contributor

Arcnor commented Apr 23, 2024

Thanks @Nakhr11n and sorry for all the requests, if you fix the conflict we can just merge this :)

@Nakhr11n
Copy link
Contributor Author

Nakhr11n commented Apr 24, 2024

Conflicts fixed. Thank you for the feedback :)

@Lgt2x Lgt2x merged commit e41e275 into DescentDevelopers:main Apr 24, 2024
@Nakhr11n Nakhr11n deleted the unzip branch April 24, 2024 09:34
JeodC pushed a commit that referenced this pull request Apr 28, 2024
Use system zlib instead of bundled.
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