Skip to content

docs(README): Suggest using completionsdir as a fallback #1379

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jtojnar
Copy link

@jtojnar jtojnar commented May 8, 2025

If the bash-completion.pc file is not available during build,
the build system would fall back to using the legacy compatdir.

Let’s update the fallback to make it consistent with the optimistic branch.

If the `bash-completion.pc` file is not available during build,
the build system would fall back to using the legacy `compatdir`.

Let’s update the fallback to make it consistent with the optimistic branch.
Copy link
Owner

Choose a reason for hiding this comment

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

Makes sense to me, but in the same answer we later refer to datarootdir instead of datadir. I've read the descriptions of the two, became confused, but was left with the feeling that datadir would actually be what we should be using (mostly because the datadir description mentions it actually being the dir for installing things into, datarootdir does not). We also use datadir for the replacements we do for the .pc file, not datarootdir.

This is not the fault of this change, but as it makes the difference so obvious, I'd like to discuss here what to do about it. I guess I'm suggesting this (to be done in another commit/PR):

--- a/README.md
+++ b/README.md
@@ -236 +236 @@ A. [ Disclaimer: Here, how to make the completion code visible to
-   bashcompdir = $(datarootdir)/bash-completion/completions
+   bashcompdir = $(datadir)/bash-completion/completions
@@ -243 +243 @@ A. [ Disclaimer: Here, how to make the completion code visible to
-   install(FILES your-completion-file DESTINATION "${CMAKE_INSTALL_DATAROOTDIR}/bash-completion/completions")
+   install(FILES your-completion-file DESTINATION "${CMAKE_INSTALL_DATADIR}/bash-completion/completions")

@akinomyoga your thoughts on this? Based on git log you were involved with the commit that added these two mentions, 530017d

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.

2 participants