Makefile.PL: set include paths correctly #213
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It turns out that the cc_include_flags function, from Module::Install::XSUtil, doesn’t like being given raw command-line arguments (e.g. a string like "-I/path/to/thing -I/some/thing/else"). We have to give it a list of strings without the prefixing "-I" (e.g. "/path/to/thing", "/some/thing/else"). This commit takes care of that.
marc-vanderwal
requested review from
MichaelTimbert,
matsduf,
mattias-p and
tgreenx
marc-vanderwal
added
the
T-Bug
Contributor
|
@marc-vanderwal, this is a draft. Is it ready for review? |
Contributor
Author
|
Not yet, I’d like to know if it works on someone else’s machine, for example @ondohotola’s setup. |
matsduf
added
the
V-Patch
Contributor
|
I have built and installed on FreeBSD without any issues. |
|
It builds on the Intel (15.1.1) without any issues. On the Silicon (15.1.1) with Homebrew (including The following works: showing: |
The cc_libs function in Module::Install::XSUtil expects a list of strings, while we were passing it a single string containing whitespace-delimited items. Split it, so that it hopefully works as intended.
Contributor
Author
|
@ondohotola Thanks for the feedback! And it’s a good thing that passing I’ve found a small mistake in my new code and pushed a fix. Does that work on ARM-based Macs too? |
|
No, does not work on ARM: |
|
I just found out that also works. Still not ideal, but one doesn't have to go into the build directory. |
When pkg-config is used to discover the appropriate header and library locations, the new code forgot to update a hash that is passed to the cc_assert_lib function later on. This means that on systems with non-traditional locations for openssl or ldns libraries, the cc_assert_lib didn’t check the locations previously discovered by pkg-config. This commit should ensure that cc_assert_lib finds those libraries when testing for their presence.
Contributor
Author
|
I agree that it isn’t ideal. Actually, I think I’ve found the problem: I forgot to update a data structure later passed on to functions in Devel::CheckLib with the information obtained from |
|
Same error on AMR, works on Intel |
Contributor
Author
|
I now have access to an ARM-based Mac, but haven’t had the time to continue investigating this problem. I’ll push the fix to the next release. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
This PR addresses an oversight in #210 that causes Makefile.PL to fail when a library needs include paths (
-I) specified on the compiler command line.Context
See:
Changes
Filter the output of
pkg-configto turn it into a list of raw paths, for consumption bycc_include_pathsfrom Module::Install::XSUtil.How to test this PR
Run Makefile.PL, ideally while having OpenSSL or LDNS installed in a custom location such as the user’s home directory.