uftrace icon indicating copy to clipboard operation
uftrace copied to clipboard

build: Relocate bash completion script

Open kangtegong opened this issue 3 years ago • 12 comments

This commit fixes bash completion feature, which is not working right now.

misc/bash_completion.sh should be relocated, and we can get recommended location of the script wq!:with pkg-config --variable=completionsdir bash-completion.

Other directories, including /usr/local/etc/bash_completions.d are only present for backwords compatibility, so its usage is no longer recommended.

Fixed: #873

Signed-off-by: Kang Minchul [email protected]

kangtegong avatar May 16 '21 16:05 kangtegong

Reference: https://github.com/scop/bash-completion#faq

Q. I author/maintain package X and would like to maintain my own completion code for this package. Where should I put it to be sure that interactive bash shells will find it and source it?

A. The recommended directory is completionsdir, which you can get with pkg-config --variable=completionsdir bash-completion.

kangtegong avatar May 16 '21 16:05 kangtegong

I cannot install with this PR when configure --prefix is used.

$ ./configure --prefix=$HOME/usr
$ make
$ make install
  INSTALL  uftrace
  INSTALL  libmcount
  INSTALL  bash-completion
install: cannot create regular file '/usr/share/bash-completion/completions/uftrace': Permission denied
Makefile:324: recipe for target 'install' failed
make: *** [install] Error 1

In addition, I don't remember how to add this bash complete file when it's installed in a custom path.

honggyukim avatar May 16 '21 17:05 honggyukim

Sure, all installation directories should honor the prefix setting and users sometimes want to install it in a non-system directory.

namhyung avatar May 17 '21 06:05 namhyung

how do you think about this commit then? This one will guarantee bash completion work correctly when sudo make installed or installed by root at least.

ifeq ($(shell id -u),0)	# check if make install was ran by root, or sudo make installed
  completionsdir = $(shell pkg-config --variable=completionsdir bash-completion)
else			# other installations, including ./configure --prefix
  completionsdir = $(etcdir)/bash-completion.d
endif

Plus, bash completion enables (or disables) when shell is refreshed, like exec $SHELL or su, etc. Not right after (sudo) make install or (sudo) make uninstall. I don't know if this is casual.

+ Thanks for code review :) You don't have to merge if this doesn't look good enough.

kangtegong avatar May 18 '21 16:05 kangtegong

I think it's better to check if the prefix is /usr instead of uid being 0. For some cases, they want to install it in a different location.

Currently it install the completion script to $(etcdir)/bash_completion.d, but you can change it to $(prefix)/$(remaining-completion-dir) if pkg-config has it.

namhyung avatar May 20 '21 06:05 namhyung

Ok, I just amended the commit like below

 # check if prefix of installation location is /usr/
ifeq ($(shell echo $(DESTDIR)$(prefix) | head -c 5),/usr/) 
# check pkg-config has bash-completion directory
ifneq ($(shell pkg-config --variable=completionsdir bash-completion),)  
	$(Q)$(INSTALL) -m 644 $(srcdir)/misc/bash-completion.sh $(shell pkg-config --variable=completionsdir bash-completion)/uftrace
endif
endif

kangtegong avatar May 20 '21 11:05 kangtegong

just amended commit for code readability. changed head to cut

 # check if prefix of installation location is /usr/
ifeq ($(shell echo $(DESTDIR)$(prefix) | cut -c 1-5),/usr/)
# check pkg-config has bash-completion directory
ifneq ($(shell pkg-config --variable=completionsdir bash-completion),)
	$(Q)$(INSTALL) -m 644 $(srcdir)/misc/bash-completion.sh $(shell pkg-config --variable=completionsdir bash-completion)/uftrace
endif
endif

kangtegong avatar May 20 '21 15:05 kangtegong

Sorry for the late reply. I think you'd better changing configure script to figure out a directory to install. Currently it detects 'etcdir' and install the script to 'etcdir/bash_completion.d' but I think you can change it to 'cmpldir' (or whatever) to include the whole path and do some conversion using pkg-config if detected.

namhyung avatar May 28 '21 19:05 namhyung

Thanks for reply :) just rebased and amended commit as you mentioned.

kangtegong avatar May 31 '21 08:05 kangtegong

i just amend the commit as @namhyung said, and clarified compldir path in configure script in simpler way.

Thanks for feedbacks!

kangtegong avatar Aug 19 '21 08:08 kangtegong

if this commit is ok, I want to leave these other issues and amend misc/bash-completion.sh.

  1. auto-suggest uftrace arguments hierarchically

  2. enable bash completion when uftrace is built with prefix given, and installed binary is not in $PATH (e.g. ./configure --prefix=$HOME/usr) because as far as I know, currently misc/bash-completion.sh is supported only when uftrace binary is in $PATH

kangtegong avatar Aug 19 '21 08:08 kangtegong

  1. enable bash completion when uftrace is built with prefix given, and installed binary is not in $PATH (e.g. ./configure --prefix=$HOME/usr) because as far as I know, currently misc/bash-completion.sh is supported only when uftrace binary is in $PATH

But I don't think it's a good idea. For the custom build version, users need to update the script. I don't want to touch it unnecessarily.

namhyung avatar Aug 21 '21 23:08 namhyung