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

Allow shared zlib-ng #11747

Merged
merged 1 commit into from Jul 16, 2023
Merged

Conversation

Mystro256
Copy link
Contributor

Should be self explanatory.

Allows for using shared zlib-ng if found by pkgconfig, similar to how zstd works.

@delroth
Copy link
Member

delroth commented Apr 10, 2023

@dolphin-emu-bot rebuild

@Mystro256
Copy link
Contributor Author

I guess I should also add "check_vendoring_approved(zlib-ng)"? I'm not sure how this logic works, so I didn't touch it.

@delroth
Copy link
Member

delroth commented Apr 11, 2023

Please do -- I started working on this to make it easier for package maintainers to notice when we add new dependencies, by having an allowlist of things that they are ok with us vendoring. The idea being that when bumping to the next package version any new vendored dependency that you haven't explicitly approved (by adding it to an allowlist in a cmake configuration flag) would cause the build to fail, instead of silently using a vendored version of the lib.

Do you maintain the Fedora package for Dolphin? If so I'd love feedback on whether that's useful / how to make it more useful :)

(It wasn't there for zlib-ng because we didn't support non-vendored.)

@delroth
Copy link
Member

delroth commented Apr 11, 2023

@Mystro256
Copy link
Contributor Author

Thanks! I noticed there's a linking error:
/usr/bin/ld: ../DiscIO/libdiscio.a(CompressedBlob.cpp.o): undefined reference to symbol 'inflateEnd'

So I guess I should have waited for the build to complete before pushing. I believe inflateEnd comes from zlib if I recall correctly.

@Mystro256
Copy link
Contributor Author

Hmm having trouble with this one... It keeps trying to link against the system libz instead of libz-ng, causing missing symbol issues.

@Mystro256
Copy link
Contributor Author

Sorry for the delay. I rebased on the latest master and it looks like dolphin_find_optional_system_library_pkgconfig resolves all my issues.

@Mystro256
Copy link
Contributor Author

@TellowKrinkle thanks for implementing this. It makes everything easier :)

@AdmiralCurtiss
Copy link
Contributor

@TellowKrinkle I assume this is fine? Do you know if we need any specific zlib-ng version?

@TellowKrinkle
Copy link
Contributor

TellowKrinkle commented Jul 16, 2023

Yes, I think this is fine. I do not know any specific zlib-ng version, and while we could try to search through to figure out, I'd be in favor of the "put it in with no minimum, and add a minimum if we get an issue about it failing to compile" solution

@AdmiralCurtiss AdmiralCurtiss merged commit dfdaeb1 into dolphin-emu:master Jul 16, 2023
11 checks passed
@AdmiralCurtiss
Copy link
Contributor

Reasonable.

@SuperSamus
Copy link
Contributor

Thanks! I noticed there's a linking error:
/usr/bin/ld: ../DiscIO/libdiscio.a(CompressedBlob.cpp.o): undefined reference to symbol 'inflateEnd'

I found out that it also happens if zlib-ng is shared, but minizip-ng is Dolphin's.
(Guess I should link #12070)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants