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

Write more variables into zlib.pc #157

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Sep 12, 2023

Added version, that is parsed from zlib.h and includedir and libdir

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks for contributing.

Could you explain what the purpose of this change is? What's the benefit, what does it fix?
Thanks

@Byron Byron added the question There is a question to be answered label Sep 12, 2023
@sagudev
Copy link
Contributor Author

sagudev commented Sep 12, 2023

Some C/C++ programs have configure scripts that check zlib versions. This PR just fills some of those placeholder so zlib.pc can be used by configure script.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thank you, I think more information shouldn't hurt.

I thought about this and have to consider it a backwards-incompatible change as this will crash hard if a header file provided by any system does not contain the desired information, information that previously wasn't provided.

To be save, please make the version optional. Thank you.

build.rs Show resolved Hide resolved
@sagudev
Copy link
Contributor Author

sagudev commented Sep 13, 2023

I thought about this and have to consider it a backwards-incompatible change as this will crash hard if a header file provided by any system does not contain the desired information, information that previously wasn't provided.

Making zlib.pc is only done when compiling from source, which is part of this repo, so zlib.h will always contain that information, that's how zlib's build system is doing it. But better safe than sorry, so there will be no panic on version fail.

@sagudev sagudev requested a review from Byron September 13, 2023 06:44
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

I am sorry for the earlier review - it was based on a browser-only session with a tiny context window. This made me miss that indeed, this change only affects the vendored version, but also that comes with zlib.pc.in which has libdir and includedir variables ready for substitution.

With that knowledge, I think the change is good the way it was before 😅, assuming that you are sure that the generated includedir and libdir variables will be correct on any system.

Thanks for bearing with me.

@sagudev
Copy link
Contributor Author

sagudev commented Sep 13, 2023

No worries, git stores previous commits too.

@sagudev
Copy link
Contributor Author

sagudev commented Sep 13, 2023

assuming that you are sure that the generated includedir and libdir variables will be correct on any system.

If prefix is valid than includedir and libdir should be valid too.

@Byron Byron merged commit c19e488 into rust-lang:main Sep 13, 2023
86 checks passed
@Byron
Copy link
Member

Byron commented Sep 13, 2023

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question There is a question to be answered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants