All of lore.kernel.org
 help / color / mirror / Atom feed
From: "John W. Linville" <linville@tuxdriver.com>
To: Kevin Locke <kevin@kevinlocke.name>,
	netdev@vger.kernel.org,
	Jesse Brandeburg <jesse.brandeburg@intel.com>
Subject: Re: [PATCH v2] ethtool: Add bash-completion script
Date: Thu, 18 Apr 2019 12:05:34 -0400	[thread overview]
Message-ID: <20190418160533.GA22913@tuxdriver.com> (raw)
In-Reply-To: <20190417025333.GA28674@kevinolos>

On Tue, Apr 16, 2019 at 08:53:33PM -0600, Kevin Locke wrote:
> On Tue, 2019-04-16 at 14:37 -0400, John W. Linville wrote:
> > Overall, it looks good to me. But when I build with "make distcheck",
> > I get this output:
> > 
> > [...]
> > 
> > It looks like somewhere you are using "$(bashcompletiondir)" instead of
> > "$(DESTDIR)$(bashcompletiondir)", but I can't seem to find it. Perhaps
> > you can find the change required to avoid this build error?
> 
> Thanks for taking a look at it!  Good catch.  DESTDIR would have been
> my guess as well.  Interestingly, the issue is that make distcheck
> expects `./configure --prefix=foo && make install` to only install
> files below foo, which fails because --with-bash-completion-dir
> defaults to `pkg-config --variable=completionsdir bash-completion`
> (usually /usr/share/bash-completion/completions).
> 
> I can think of a few different ways to fix this:
> 
> 1. Add =--without-bash-completion-dir or
>    --with-bashcompletiondir=$something to DISTCHECK_CONFIGURE_FLAGS
>    in Makefile.am to avoid installing the script during distcheck.
> 2. Replace the prefix for bash-completion (using
>    `pkg-config --variable=prefix bash-completion`) in
>    $bashcompletiondir with --prefix passed to configure.
> 3. Stop using pkg-config and install to
>    $datadir/bash-completion/completions by default.
> 
> Option 1 has the disadvantage that users may not expect files to be
> installed outside of --prefix, that the script does not install to
> /usr/local (with everything else) by default, and that
> --with-bash-completion-dir= must be passed for non-root installs.
> kmod passes
> --with-bashcompletiondir=$$dc_install_base/$(bashcompletiondir)
> to DISTCHECK_CONFIGURE_FLAGS, which has the additional disadvantage of
> using the undocumented (AFAICT) Automake $$dc_install_base variable.
> 
> Options 2 and 3 have the disadvantage that passing --prefix= which is
> not the prefix of $XDG_DATA_HOME or $XDG_DATA_DIRS will install the
> script to a directory that bash-completion doesn't use.
> 
> Option 3 has the additional disadvantage of ignoring the upstream
> recommendations, which could install the script to a directory not
> used by bash-completion for customized or future versions.  It has the
> advantage of being extremely simple and understandable.
> 
> My personal preference is #2, but I would defer to your judgement.
> Let me know which you would prefer and I'll update the patch.
> 
> Best,
> Kevin
> 
> P.S.  I noticed that the PKG_CHECK_MODULES macro does unnecessary work
> handling BASH_COMPLETION_CFLAGS and BASH_COMPLETION_LIBS and adds them
> to the configure --help text, so I will remove it in favor of calling
> $PKG_CONFIG directly, unless you object.

Option #2 seems reasonable, and your "P.S." seems fine too.

Thanks,

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

  reply	other threads:[~2019-04-18 16:15 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-08  2:19 [PATCH] ethtool: Add bash-completion script Kevin Locke
2019-04-11 16:14 ` Jesse Brandeburg
2019-04-11 16:47   ` Kevin Locke
2019-04-11 17:39 ` [PATCH v2] " Kevin Locke
2019-04-16 18:37   ` John W. Linville
2019-04-17  2:53     ` Kevin Locke
2019-04-18 16:05       ` John W. Linville [this message]
2019-04-20  0:16   ` [PATCH v3] " Kevin Locke
2019-04-24 19:54     ` John W. Linville

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190418160533.GA22913@tuxdriver.com \
    --to=linville@tuxdriver.com \
    --cc=jesse.brandeburg@intel.com \
    --cc=kevin@kevinlocke.name \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.