-
-
Notifications
You must be signed in to change notification settings - Fork 155
support installing to absolute paths #155
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
Conversation
WalkthroughUpdated libchromaprint.pc.cmake to use absolute install paths for exec_prefix, libdir, and includedir via CMAKE_INSTALL_FULL_* variables. prefix remains CMAKE_INSTALL_PREFIX. No other lines changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
libchromaprint.pc.cmake(1 hunks)
🔇 Additional comments (2)
libchromaprint.pc.cmake (2)
3-4: LGTM on switching to FULL_ for absolute paths.*Using @CMAKE_INSTALL_FULL_LIBDIR@ and @CMAKE_INSTALL_FULL_INCLUDEDIR@ solves the absolute install path need (e.g., nix) and keeps Libs/Cflags correct.
3-4: No action needed: include(GNUInstallDirs) is present in CMakeLists.txt (line 24) and cmake_minimum_required(VERSION 3.10) ≥ 3.4, which introduced CMAKE_INSTALL_FULL_* variables. (manpages.debian.org)
|
@jopejoe1 Explain this to me, please. You are only changing the pkg-config file, and in there, it's standard to define the prefix as absolute path, and then use the prefix for other paths. Why do you need absolute paths there? It seems that you would only need them if you are using some broken parser for the pkg-config files, as pkg-config itself should be able to evaluate this correctly. Maybe you wanted to change where the files are actually installed? What do you need that you can't achieve by just setting the |
|
That would generate a broken path in the case where |
|
@jopejoe1 why don't you use the prefix and set that to absolute path? can you show me how you build other cmake projects? |
|
Oh, I see, you use different prefixes for include dirs than the library, NixOS/nixpkgs#144170 (comment) Well, I don't like this, but I think it doesn't hurt to change it, so I'll do it. |
Using
@CMAKE_INSTALL_FULL_*DIR@instead of using${prefix}/@CMAKE_INSTALL_*DIR@makes it possible to install to absolute paths and not just relative paths. Which we need in nixpkgs to correctly build the package.