Add functions and "snippets" hierarchy #2498

Closed
wants to merge 5 commits into
from

Projects

None yet

6 participants

@faho
Member
faho commented Oct 20, 2015

This allows "vendors" (i.e. third-party upstreams interested in
supporting fish) to add auto-loaded functions and eager-loaded
"snippets", while still allowing both the user and the administrator to
fully override all of that.

This has been inspired by systemd's configuration hierarchy.

Fixes #1956

This is a slightly-less-quick alternative to #2496. It's probably not ready (the terms aren't final either), it's just meant to get the conversation going again.

@faho faho Add functions and "snippets" hierarchy
This allows "vendors" (i.e. third-party upstreams interested in
supporting fish) to add auto-loaded functions and eager-loaded
"snippets", while still allowing both the user and the administrator to
fully override all of that.

This has been inspired by systemd's configuration hierarchy.

Fixes #1956
91ea1ff
@faho faho referenced this pull request Oct 20, 2015
Closed

Source sys and data configs #2496

@faho
Member
faho commented Oct 20, 2015

Currently this doesn't work because $XDG_CONFIG_HOME isn't defined in config.fish yet - any idea what we should do?

@pickfire
Contributor

Maybe use something like others app would do:

  1. Check ~/.config/*
  2. Check $XDG_CONFIG_HOME
@faho
Member
faho commented Oct 21, 2015

Maybe use something like others app would do:

That's actually the behavior documented in the spec. I was confused because I assumed we'd inherit an XDG_CONFIG_HOME, but if we did it would already be defined.

@davispuh

Looks good to me. Maybe fish should set all XDG envs? because if it's started as shell after login on TTY there won't be anything that sets them.

@faho
Member
faho commented Oct 21, 2015

Turns out we already did the "XDG_CONFIG_HOME or ~/.config" dance above - though only locally. But this isn't the place to define XDG__ always (that'd be a separate PR and discussion) or make a "$__fish_userconfdir" global variable (like "$__fish_sysconfdir").

@jrobeson

this is really good here. It'd be nice to be able to boot with an empty /etc/ but still have be able to use vendor provided fish config , instead of relying in /etc/fish

@jrobeson

$_fish_sysconfdir sounds reasonable. I wouldn't mind if the XDG* vars were set by fish (if not previously set) though!

@zanchey
Member
zanchey commented Dec 11, 2015

I don't think fish should set the XDG variables; my reading of the XDG spec suggests that we would be responsible for maintaining them including checking permissions and maintaining a lifecycle, which I don't think is fish's role.

I think it's reasonable to set $__fish_userconfdir. The reason I wasn't terribly keen on it is that if you set $XDG_CONFIG_HOME (or whatever) then the variable won't automatically be updated (though a handler could be added to do that), but then it turns out that fish doesn't actually react to changes in those variables anyway.

@zanchey
Member
zanchey commented Dec 11, 2015

@faho, this looks good. At the risk of bikeshedding, I think conf.d is a better name than snippets.d, and is more in keeping with the way that at least Debian packages work.

It will definitely need noting in the documentation.

r+ zanchey@ with the conf.d change, although of course I would appreciate the opinion of other packagers such as @amluto.

@zanchey zanchey added this to the next-2.x milestone Dec 11, 2015
@zanchey zanchey commented on an outdated diff Dec 11, 2015
share/config.fish
@@ -152,3 +152,15 @@ function . --description 'Evaluate contents of file (deprecated, see "source")'
source $argv
end
end
+
+# As last part of initialization, source the snippets directories
+# Implement precedence (User > Admin > Vendors > Fish) by basically doing "basename"
+set -l sourcelist
+for file in $configdir/fish/snippets/* $__fish_sysconfdir/snippets/* $__fish_datadir/vendor_snippets.d/*
@zanchey
zanchey Dec 11, 2015 Member

This is pretty clever, but we'll have to watch the case where those wildcards expand to nothing once #2394 is changed.

@zanchey zanchey commented on an outdated diff Dec 11, 2015
@@ -1,6 +1,8 @@
prefix=@prefix@
datadir=@datadir@
completionsdir=${datadir}/fish/vendor_completions.d
+functionsdir=${datadir}/fish/vendor_functions.d
+snippetsdir=${datadir}/fish/vendor_snippets.d
@zanchey
zanchey Dec 11, 2015 Member

We need to keep Homebrew in view here - these should all be separate configure-able variables with sensible defaults. I'm happy to fix that later as part of #2113.

@zanchey zanchey self-assigned this Dec 11, 2015
@amluto
Contributor
amluto commented Dec 11, 2015

This looks sensible to me, but I think I slightly prefer "conf.d" over "vendor-snippets.d".

@faho
Member
faho commented Dec 11, 2015

@amluto: Just "conf.d" or "vendor-conf.d"? I think the latter'd have a nice symmetry with "completions"/"vendor-completions.d".

@amluto
Contributor
amluto commented Dec 11, 2015

If it weren't for all the others, I'd say just "conf.d". But "vendor-conf.d" would be fine, too.

@faho faho Rename "snippets" to "conf.d"
Seems more intuitive. Also more consistent with the ".d" prefix.
0021663
@faho
Member
faho commented Dec 11, 2015

@zanchey, @amluto: Are both of you happy now?

@amluto
Contributor
amluto commented Dec 12, 2015

Looks good to me.

Is there anything that moves /etc/fish/config.fish into /usr/share? If not, I can do it in the Fedora package.

@zanchey
Member
zanchey commented Dec 16, 2015

There won't be, no - I think that's best left to the packaging infrastructure as it will require superuser permissions and won't work well either in fish launch or the Makefile.

@faho
Member
faho commented Dec 16, 2015

@amluto, @zanchey: How about appending everything in etc/config.fish to share/config.fish (which seems to be the same order we currently have) and then removing it entirely? $__fish_sysconfdir/config.fish would still be available, only not installed by default.

Or moving it to conf.d would be fine, that would keep it overrideable. Though I don't quite like that file - is /etc/sysconfig/i18n still a thing? That's a Fedora-ism IIRC, and you guys switched over to systemd's locale.conf a while ago. (It also contains a useless-use-of-cat, and a use of sed in the startup path, which is why I'd like to remove it)

@amluto
Contributor
amluto commented Dec 17, 2015
@faho faho Add conf.d documentation
It's a bit tough to find the balance between giving packagers and
third-party programmers good information but not overwhelming the casual user.
3fd9e7c
@faho
Member
faho commented Dec 17, 2015

@amluto: We'll do that afterwards. How about /etc/sysconfig/i18n? Do you know of anyone still using that?

@zanchey: I've now attempted some documentation. It's probably not ideal (as noted, finding the right balance here is tough), but should explain it alright.

@amluto
Contributor
amluto commented Dec 17, 2015

I have no clue about /etc/sysconfig/i18n. I could ask around, I guess.
On Dec 17, 2015 3:25 AM, "Fabian Homborg" notifications@github.com wrote:

@amluto https://github.com/amluto: We'll do that afterwards. How about
/etc/sysconfig/i18n? Do you know of anyone still using that?

@zanchey https://github.com/zanchey: I've now attempted some
documentation. It's probably not ideal (as noted, finding the right balance
here is tough), but should explain it alright.


Reply to this email directly or view it on GitHub
#2498 (comment)
.

@faho faho removed the needsdocs label Dec 29, 2015
@faho
Member
faho commented Dec 29, 2015

@zanchey: Merge at your discretion - I can't see anything missing.

@jrobeson jrobeson commented on the diff Feb 9, 2016
share/config.fish
@@ -152,3 +152,15 @@ function . --description 'Evaluate contents of file (deprecated, see "source")'
source $argv
end
end
+
+# As last part of initialization, source the conf directories
+# Implement precedence (User > Admin > Vendors > Fish) by basically doing "basename"
+set -l sourcelist
+for file in $configdir/fish/conf.d/* $__fish_sysconfdir/conf.d/* $__fish_datadir/vendor_conf.d/*
@jrobeson
jrobeson Feb 9, 2016

shouldn't $__fish_sysconfdir/conf.d itself be allowed to not exist? and all of $__fish_sysconfdir for that matter. I've been experimenting with empty /etc (stateless systems), and this seems like it would break that.

@faho
faho Feb 9, 2016 Member

@jrobeson: This actually works in that case because the glob currently expands to nothing (try echo /arglbargl/* in a script - interactively it'll warn, but in a script it'll work). With #2719 as solution to #2394 it will continue to work. Yes, if we go with something else it might need to be revisited.

@faho
Member
faho commented Feb 13, 2016

If there are no further comments, I intend to merge this in a week.

@ghost
ghost commented Feb 13, 2016

Please correct me if I am wrong, I am trying to understand exactly what these changes mean.

So, I see users will be able to add their own "initialization" code to ~/.config/fish/conf.d which fish will now run during its own initialization process.

But other than that, I am not sure what you mean by eager-loaded "snippets" and how, if at all, this affects what functions are autoloaded.

@faho
Member
faho commented Feb 13, 2016

But other than that, I am not sure what you mean by eager-loaded "snippets" and how, if at all, this affects what functions are autoloaded.

Those are the files in conf.d. They can of course affect what functions are autoloaded - they are full fish scripts that are just sourced, after all, so they could e.g. alter fish_function_path. You'll want to be careful with these, but if anyone writing these snippets screws up, it's easy enough to override and report a bug to them.

@ghost
ghost commented Feb 13, 2016

@faho Sure. But what I mean is: other than these new "initialization" files is there anything else I should know about the upcoming changes that I may be missing?

I was just confused of what you meant by "snippets" as I prefer to think of them a "initialization" files, but maybe that's just me.

@faho
Member
faho commented Feb 13, 2016

But what I mean is: other than these new "initialization" files is there anything else I should know about the upcoming changes that I may be missing?

@bucaran: This touches three kinds of files:

  • Functions
  • "Snippets"/"Fragments"

The change for functions is rather trivial: There's now an additional directory for third-party (i.e. neither fish nor the user) "vendor functions", just like the one for vendor completions. I'm not sure if it'll be useful to you as it'll be system-wide and most likely root-owned. These are useful for other programs, especially those installed via package manager.

"Snippets" are introduced here - these are fish scripts that are supposed to be sourced. Like functions and completions, there are four directories for the user, administrator, third-parties and fish, respectively. And also like functions and completions, for every filename, fish will only (attempt to) read the first one that exists. If you have files with the same name in the user and the administrator directories, the user wins.

The main use-case for these was something like pantheon-terminal, which supports completion notifications via dbus - see #1382.

@ghost
ghost commented Feb 13, 2016

Gotcha.

So, what's up with snippet? I am not familiar with the terminology in this context.

@faho
Member
faho commented Feb 13, 2016

Not my terminology - it's called that in systemd, xorg and potentially other places, with roughly the same semantics.

@ghost
ghost commented Feb 13, 2016

Good to know and thanks 👍

@zanchey
Member
zanchey commented Feb 14, 2016

Some more history is at https://lists.debian.org/debian-devel/2010/04/msg00352.html.

I think fragments is better than snippets but taking precedent from systemd seems reasonable #bikeshed

@ghost
ghost commented Feb 14, 2016

Fragments? I am really glad we're going with snippets now.

@zanchey zanchey referenced this pull request Feb 25, 2016
Closed

Cut a new release #2760

@faho
Member
faho commented Feb 26, 2016

Merged as c1b384e. Thanks for all the comments!

@faho faho closed this Feb 26, 2016
@faho
Member
faho commented Mar 26, 2016

For whoever'll draft the release notes: After discussion with @zanchey, I've now (7accadc) refined this to only read .fish files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment