Skip to content

CMake: preserve absolute pkg-config install directories - #264

Merged
dbry merged 1 commit into
dbry:masterfrom
fhgffy:fix/pkgconfig-absolute-paths
Oct 6, 2026
Merged

dbry merged 1 commit into
dbry:masterfrom
fhgffy:fix/pkgconfig-absolute-paths

Conversation

@fhgffy

@fhgffy fhgffy commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem

When CMAKE_INSTALL_LIBDIR or CMAKE_INSTALL_INCLUDEDIR is absolute, CMake installs to that directory but wavpack.pc prepends ${prefix}/. For example, prefix /tmp/wv-prefix and libdir /tmp/wv-lib incorrectly produce /tmp/wv-prefix//tmp/wv-lib, so downstream pkg-config consumers cannot find the installed files.

Change

Keep absolute values when generating the .pc file and retain the existing ${prefix} expressions for relative values. The final diff contains only six conditional lines in CMakeLists.txt, following the review request to omit the configuration test and its CI/distribution wiring.

Validation

After the scope reduction, five local CMake configuration cases pass: default, custom relative, absolute library only, absolute include only, and both absolute directories. All ten generated lib/include fields match the actual configured paths. git diff --check passes, and the aggregate diff against b6485a3 contains only CMakeLists.txt (+6/-0).

Earlier validation of this unchanged CMake production change included installed static consumers, relative prefix overrides, the pkg-config-module-off option, and the enabled program/CTest builds. After squashing, a fresh Linux Debug build with programs and BUILD_TESTING enabled also passes, and CTest passes wvtest (1/1). The full source tree is identical to the previous three-commit head. The regression is retained privately for verification and is no longer part of this PR. Windows and macOS were not run locally; current remote CI is separate.

@fhgffy

fhgffy commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the regression in 14a9aed: its default build directory now sits outside the source tree, and CMake cache entries are read as UTF-8. The original root-directory command, cmake -P cmake/tests/pkgconfig-paths.cmake, passes all five configuration cases with CMake 3.28.3 on Linux, including the UTF-8 paths in this workspace.

@dbry

dbry commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Thanks for the contribution!

Unfortunately I'm not that familiar with the cmake stuff (other than using it and basic edits) so I'd like to loop in the authors @sezero and @madebr . If they have no comments I'm happy to pull this in.

@sezero

sezero commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

The change to CMakeLists.txt looks fair to me.

@madebr should look at the whole thing if he's able.

Comment thread .github/workflows/build.yml Outdated
run: git config --global --add safe.directory ${GITHUB_WORKSPACE}
- name: Configure
run: cmake -S . -B build -DCMAKE_BUILD_TYPE="Debug" -DBUILD_SHARED_LIBS=ON -DWAVPACK_BUILD_PROGRAMS=ON
- name: Check pkg-config install directories

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This step runs CMake configuration again.

I think this does not need a test and only the CMakeLists.text change should be kept.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the configuration test and its CI/dist wiring in f463138. The PR now changes only the six lines in CMakeLists.txt. I kept the regression locally and reran the five directory configurations; all ten generated path fields pass.

@sezero

sezero commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Please squash your commits into a single one.

Keep explicit absolute library and include paths in wavpack.pc while
retaining prefix-relative paths for relative install directories.
@fhgffy

fhgffy commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Would a squash merge on your side work for keeping this as one commit? The final diff is now just the six-line CMakeLists.txt change.

@fhgffy
fhgffy force-pushed the fix/pkgconfig-absolute-paths branch from f463138 to f1690e0 Compare October 6, 2026 18:13
@fhgffy

fhgffy commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Squashed the three commits into f1690e0. The diff is unchanged: only six added lines in CMakeLists.txt. I reran all five pkg-config directory configurations, the Debug build, and CTest (wvtest); all passed.

@dbry
dbry merged commit 18b2462 into dbry:master Oct 6, 2026
10 checks passed
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