Skip to content

Use /etc as sysconfdir for install prefix /usr #1856

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

Merged
merged 3 commits into from
Aug 20, 2021

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Aug 19, 2021

📔 Description

Per the recommendation at
https://cmake.org/cmake/help/v3.21/module/GNUInstallDirs.html#special-cases

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Install static binary to /usr/bin/vast and test locally whether /etc/vast/vast.yaml is getting picked up.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Aug 19, 2021
@@ -60,6 +60,8 @@ std::filesystem::path install_datadir() {
std::filesystem::path install_configdir() {
#if VAST_ENABLE_RELOCATABLE_INSTALLATIONS
const auto [prefix, _] = install_prefix_and_suffix();
if (prefix == "/usr")
return "/@CMAKE_INSTALL_SYSCONFDIR@/vast";
Copy link
Member

Choose a reason for hiding this comment

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

Usually this is solved at the configury-level (ie. debian packages are usually built with --prefix=/usr --sysconfdir=/etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for relocatable binaries, which do not have build-time configured paths.

Copy link
Member

Choose a reason for hiding this comment

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

It seems strange to use configuration time install paths in a relocatable binary, do we even set them?

Suggested change
return "/@CMAKE_INSTALL_SYSCONFDIR@/vast";
return "/etc/vast";

Copy link
Member Author

Choose a reason for hiding this comment

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

We use them as relocatable paths, e.g., on some systems lib64 is used over lib and we want to support relocatable binaries for those systems as well.

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually want this special case if the user has specified a custom SYSCONFDIR? The logic from the PR title (prefix /usr -> sysconfdir /etc) is now subtly different than whats implemented (prefix /usr -> prefix is ignored for sysconfdir)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. So probably hardcoded /usr -> /etc it is.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's possible to know whether to use lib or lib64 at configuration time either. We explicitly support "relocating" the binary to other systems after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's possible to know whether to use lib or lib64 at configuration time either. We explicitly support "relocating" the binary to other systems after all.

You're misunderstanding. I was saying that we do this relative dir magic in order not to hardcode libdir etc, because they may be relative paths that are not the defaults.

Co-authored-by: tobim <tobim@fastmail.fm>
@tobim
Copy link
Member

tobim commented Aug 20, 2021

I guess you can undraft this now.

@dominiklohmann dominiklohmann marked this pull request as ready for review August 20, 2021 09:59
@dominiklohmann dominiklohmann merged commit c8a651f into master Aug 20, 2021
@dominiklohmann dominiklohmann deleted the topic/special-case-usr branch August 20, 2021 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants