netdev.vger.kernel.org archive mirror
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).