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

Fix double prefix when building with cmake #723

Merged
merged 1 commit into from Jun 25, 2023
Merged

Fix double prefix when building with cmake #723

merged 1 commit into from Jun 25, 2023

Conversation

ilya-fedin
Copy link
Contributor

@ilya-fedin ilya-fedin commented Jun 15, 2022

No description provided.

@ilya-fedin
Copy link
Contributor Author

@Cyan4973 ping

@t-mat
Copy link
Contributor

t-mat commented Sep 2, 2022

@ilya-fedin, I'm sorry about these annoying errors!
#736 will fix this issue.

Copy link

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

From a quick eyeball this change looks correct to me and properly handles the various edge cases under cmake.

@Cyan4973
Copy link
Owner

Cyan4973 commented Jun 24, 2023

I'm trying to understand what was the objective of this PR.

The title mentions "Fix double prefix when building with cmake",
which implies there's an issue when building with cmake.
It's probably not a trivial scenario, since I could not detect any issue from basic cmake tests.

If that's the case, I would expect it should be possible to create a test case that reproduces this issue. Such test case could then be integrated into CI.

Looking at the PR's content, it appears focused on pkg-config generation.
More specifically includedir and libdir are now set entirely from the build system, either make or cmake, instead of being derived from prefix within the pkgconfig file. The new design seems superficially similar to the original one, but I presume it differs in subtle ways, in order to fix some edge cases that were not properly covered.
And since the new design is also a bit more complex, which is a cost to maintenance, I would like to understand precisely what it does better.

Note : I see that one test was failing, but my understanding is that the problem was on the test side, not the PR. We can ignore it.

@ilya-fedin
Copy link
Contributor Author

Try to build xxHash with the following cmake arguments:

cmake -Bbuild -DCMAKE_INSTALL_PREFIX=/usr -DCMAKE_INSTALL_INCLUDEDIR=/usr/include -DCMAKE_INSTALL_LIBDIR=/usr/lib

This is valid for cmake but xxHash's build scripts generate pkg-config file in a way it gets /usr/usr/include, /usr/usr/lib

@Cyan4973
Copy link
Owner

Cyan4973 commented Jun 25, 2023

OK !
Following the proposed test,
I get the following content in libxxhash.pc :

prefix=/usr
exec_prefix=${prefix}
includedir=${prefix}//usr/include
libdir=${exec_prefix}//usr/lib

which is obviously incorrect, and will lead to duplicated prefix /usr.

In contrast, after your patch, the content of libxxhash.pc becomes :

prefix=/usr
exec_prefix=${prefix}
includedir=/usr/include
libdir=/usr/lib

It's probably worth making a test from this example.

@ilya-fedin
Copy link
Contributor Author

Sorry, but I don't have that much interest to create a test. If this is required to get the PR merged, I'd just close it and let someone else to do the work.

@Cyan4973
Copy link
Owner

Cyan4973 commented Jun 25, 2023

Don't worry, I'm taking care of it (#845).

@Cyan4973 Cyan4973 merged commit 17bbdee into Cyan4973:dev Jun 25, 2023
Cyan4973 added a commit that referenced this pull request Jun 25, 2023
ensure proper path after advanced example provided by @ilya-fedin :
#723 (comment)

note : is expected to fail before integration of #723
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.

None yet

5 participants