All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kris Van Hees <kris.van.hees@oracle.com>
To: Nick Alcock <nick.alcock@oracle.com>
Cc: Kris Van Hees <kris.van.hees@oracle.com>,
	dtrace@lists.linux.dev, dtrace-devel@oss.oracle.com
Subject: Re: [DTrace-devel] [PATCH 0/9] relocatable DTrace
Date: Wed, 31 Jul 2024 16:11:13 -0400	[thread overview]
Message-ID: <ZqqaYbavY9AMUiEP@oracle.com> (raw)
In-Reply-To: <87a5hxmj5n.fsf@esperi.org.uk>

On Wed, Jul 31, 2024 at 08:54:44PM +0100, Nick Alcock wrote:
> On 31 Jul 2024, Kris Van Hees said:
> 
> > General comments because my review covers the entire series...
> >
> > Why are there two pkg_config files being introduced when it seems like a
> > single dtrace.pc one would be sufficient.  The dtrace utility is used both
> > for tracing and for the building of libraries and executables with USDT
> > probes,  I see no reason why a single dtrace.pc couldn't cover both.
> 
> Simply because there are two distinct use cases here that require a
> different set of libraries and headers from different places: "we want
> to use USDT using <sdt.h>", and "we are a DTrace consumer". Almost
> nothing wants both, only one wants a library, etc etc.
> 
> Bear in mind that more-or-less universal usage of pkg-config (as in,
> with CMake, Autoconf and Meson it is hard to impossible to do anything
> else) has the caller appending $(pkg-config --cflags $pkg) to CFLAGS or
> CPPFLAGS and $(pkg-config --libs $pkg) to LIBS. If we tried to use the
> same pkg-config file, this would mean that *raw USDT probe users* would
> find themselves linking with libdtrace! This is *surely* not what we
> want.

If people do that without actually checking whether it makes any sense, then
that would be an argument (to me) to not use pkg-config at all.  Using
pkg-config to get --libs when there is no library to link with is pointless
and merely shows that the person using this has no diea what they are doing.

The cflags argument I can accept, but then perhaps we need *3* pkg-config
files?  What exactly do we need this for?  Testsuite needs to know where to
find dtrace, so that could need a pkg-config file.  Anyone wanting to use
libdtrace would need to know where to find the library and what cflags to
use (finding the include files), which would need a pkg-config file.  And
compiling code with SDT would need another pkg-config file.

This brings it back to my original thought that if we are going to provide
pkg-config, we need to carefully think about what we need to provide and do
it consistently.

> pkg-config is not meant to be "one pkg-config file per package", despite
> the name: a huge number of packages have multiple. (The worst offender
> on my system appears to be Qt 6, with 136, but even a simple thing like
> xcb-util has four.)
> 
> > On that note, why can't pkg-config then also be used as a means to get the
> > location of the dtrace executable?  Why go through the trouble of performing
> > text substitution on runtest.sh to insert the location of dtrace when you
> > already use pkg-config in it to get the appropriate include directory for
> > the installed case.
> 
> Because I didn't think of it. That's an excellent idea. Adjusting
> accordingly. Will send a v2 of the whole series (because it's been
> ages).
> 
> > I think this series needs to be reworked to be consistent.  If we are going
> > to use pkg-config, then let's use it to its fullest extent rather than using
> > it for some stuff but then still hardcoding other things.  That makes no sense
> > to me.
> 
> Agreed. Anything else you can think of that's hardcoded? I can't
> immediately think of anything (because we can't use pkg-config in
> dtprobed.service, it's not a program) but you looked at this much more
> recently than me...

  reply	other threads:[~2024-07-31 20:11 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-31 18:59 [PATCH 0/9] relocatable DTrace Nick Alcock
2024-05-31 18:59 ` [PATCH 1/9] spec: install sdt*.h in /usr/lib64/dtrace/include/sys Nick Alcock
2024-05-31 18:59 ` [PATCH 2/9] build: initial pkg-config support Nick Alcock
2024-05-31 18:59 ` [PATCH 3/9] build: track configured vars Nick Alcock
2024-05-31 18:59 ` [PATCH 4/9] build: --bindir is supposed to be equivalent to --sbindir Nick Alcock
2024-05-31 18:59 ` [PATCH 5/9] build: the TESTDIR is relative to the LIBDIR by default Nick Alcock
2024-05-31 18:59 ` [PATCH 6/9] build: add a pkg-config file for dtrace consumers: use it Nick Alcock
2024-05-31 18:59 ` [PATCH 7/9] tests: delete the kernel build dir stuff Nick Alcock
2024-07-31 19:22   ` [DTrace-devel] " Kris Van Hees
2024-07-31 19:32     ` Nick Alcock
2024-05-31 18:59 ` [PATCH 8/9] build: make dtrace and dtprobed relocatable Nick Alcock
2024-05-31 19:46   ` [DTrace-devel] " Nick Alcock
2024-05-31 18:59 ` [PATCH 9/9] test: work when relocated Nick Alcock
2024-07-31 19:21 ` [DTrace-devel] [PATCH 0/9] relocatable DTrace Kris Van Hees
2024-07-31 19:54   ` Nick Alcock
2024-07-31 20:11     ` Kris Van Hees [this message]
2024-08-01 16:45       ` Nick Alcock

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=ZqqaYbavY9AMUiEP@oracle.com \
    --to=kris.van.hees@oracle.com \
    --cc=dtrace-devel@oss.oracle.com \
    --cc=dtrace@lists.linux.dev \
    --cc=nick.alcock@oracle.com \
    /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.