On Thu, Mar 21, 2024 at 09:25:36PM +0100, Mark Wielaard wrote:
> Hi,

Hi! Thanks for the review! Comments below and revision coming up :)

> On Thu, Mar 21, 2024 at 04:04:11PM +0100, Frederik “Freso” S. Olesen wrote:
> > diff --git a/config/ChangeLog b/config/ChangeLog
> > index ce1f74f6..7d88c071 100644
> > --- a/config/ChangeLog
> > +++ b/config/ChangeLog
> > @@ -1,3 +1,8 @@
> > +2024-03-21  Frederik “Freso” S. Olesen  <freso@freso.dk>
> > +
> > +	* profile.fish.in: Set $DEBUGINFOD_URLS in fish shells.
> > +	* Makefile.am: Include profile.fish in install and uninstall targets.
> 
> Since we have been putting the ChangeLog entry into the commit message
> it doesn't need to also go into the actual Changelog file.

Yeah, I wasn’t sure since it seemed like some recent(ish) commits still
put in ChangeLog entries, but CONTRIBUTORS said not to… so I opted to
just include this and it would be easy to either exclude when applying
the patch or remove in a revision. I’ll do the latter. :)

> > <snip>
> > +	$(INSTALL_DATA) profile.fish -D $(DESTDIR)$(datadir)/fish/vendor_conf.d/debuginfod.fish
> > <snip>
> > +	rm -f $(DESTDIR)$(datadir)/fish/vendor_conf.d/debuginfod.fish
> 
> Right, with --prefix=/usr that expands to
> /usr/share/fish/vendor_conf.d which seems to match the default install
> location you pointed out above. Good.

Yeah, I had to look up the directory variables to figure out where to put
it, to not just go for the “lazy” way of placing it in /etc:
https://www.gnu.org/prep/standards/html_node/Directory-Variables.html

> > diff --git a/config/profile.fish.in b/config/profile.fish.in
> > new file mode 100644
> > index 00000000..34b1ab85
> > --- /dev/null
> > +++ b/config/profile.fish.in
> > @@ -0,0 +1,14 @@
> > +# $HOME/.profile* or similar files may first set $DEBUGINFOD_URLS.
> > +# If $DEBUGINFOD_URLS is not set there, we set it from system *.url files.
> > +# $HOME/.*rc or similar files may then amend $DEBUGINFOD_URLS.
> > +# See also [man debuginfod-client-config] for other environment variables
> > +# such as $DEBUGINFOD_MAXSIZE, $DEBUGINFOD_MAXTIME, $DEBUGINFOD_PROGRESS.
> > +
> > +if not set --query DEBUGINFOD_URLS
> > +    # Use local variables so we don't need to manually unset them
> > +    set --local prefix="@prefix@"
> > +    set --local DEBUGINFOD_URLS (cat /dev/null "@sysconfdir@/debuginfod"/*.urls 2>/dev/null | string replace '\n' ' ')
> > +    if test -n "$DEBUGINFOD_URLS"
> > +        set --global --export DEBUGINFOD_URLS "$DEBUGINFOD_URLS"
> > +    end
> > +end
> 
> I don't know fish, but this looks OK.

I’ve been running this locally (but under $XDG_CONFIG_HOME)
since yesterday and it works well for me, with elfutils.urls
and archlinux.urls under /etc.

That said, I just spotted a mistake in my code, which I have missed
because that line is removed in my locally running version:
Fish’s `set` doesn’t use `=` for assignment, so the `$prefix` assignment
needs a tiny fix too.

> Note that to turn this profile.fish.in into profile.fish you need to
> mark it as a config file in configure.ac:

Ah! I’ve been having some SIGILL problems with `git grep`[1] so I missed
looking for other places where the other profile.*sh’s were mentioned.

[1] Incidentally, the attempt at debugging this issue is how I realised
that $DEBUGINFOD_URLS was _not_ automatically populated in fish… which
led me down the rabbit hole ending in this patch. 😅

> Could you sent a new patch with those two changes?

Yes, coming right up!

-- 
Solidarity,
Frederik “Freso” S. Olesen <https://freso.dk/>