zsh: patch _setxkbmap completion script
#46152
Conversation
|
Success on x86_64-linux (full log) Attempted: zsh Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: zsh Partial log (click to expand)
|
|
I'm not sure if we want to add setxkbmap to the closure. Maybe use /run/current-system/sw ? |
|
As stated in #46025 I'm not that happy about this either. However I doubt that |
|
@matthewbauer another idea I just had is to write a I'm absolutely not sure about this, it's possibly the easiest alternative to simply |
|
Does it need the full path to setxlbmap? Maybe we can just assume it’s on the path |
|
Yes, for now. And I'm not sure if I know of all possible consequences when linking these paths as well. |
|
Linking |
Ma27
force-pushed
the
fix-setxkbmap-completion
branch
from
7299cd6
to
613406b
Compare
|
@michaelpj @matthewbauer would you mind having another look at this? :) |
|
Success on x86_64-linux (full log) Attempted: zsh Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: zsh Partial log (click to expand)
|
| - for dir in /usr/lib/X11/xkb /usr/share/X11/xkb; do | ||
| - if [ -d $dir ] ; then | ||
| - sourcedir=$dir | ||
| + for dir in $(echo ${XDG_DATA_DIRS:gs/:/ /}); do |
There was a problem hiding this comment.
This is bash gibberish to me - I'm assuming you got this from someone else who traverses XDG_DATA_DIRS sensibly in bash?
There was a problem hiding this comment.
you're right. The thing is, in bash IFS=":" would solve the problem. However this is zsh and I didn't manage to get this approach working, sorry :/
Do you have a better/cleaner idea? :)
There was a problem hiding this comment.
I have no idea I'm afraid - surely there's someone else we can copy? I'm just aware that this sort of thing is easy to do wrong.
There was a problem hiding this comment.
I just learned that IFS can be used in zsh if setopt sh_word_split is set. Do you prefer the current change over the original solution?
If so, file it upstream, but tomorrow, I need to catch some sleep now.
You're right. This patch isn't nixos-specific anymore. I'll consider filing it upstream when we have agreed on a solution on how to iterate over |
Ma27
force-pushed
the
fix-setxkbmap-completion
branch
from
613406b
to
0f25d11
Compare
|
Success on aarch64-linux (full log) Attempted: zsh Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: zsh Partial log (click to expand)
|
|
LGTM (but I don't have merge rights) |
|
@michaelpj FYI, patched filed upstream: https://www.zsh.org/mla/workers/2018/msg01288.html @matthewbauer thanks to the help of @michaelpj we have a better solution for the issue. What do you think about this? :) |
Instead of searching `/usr` it should search for the `xkb`,
$XDG_DATA_DIRS will be searched. With this approach we allow compliance
on NixOS and non-NixOS systems to find `symbols` in the `xkb` directory.
The patch has been accepted by upstream, but isn't released yet, so this
is mainly a temporary fix until we can bump ZSH to the next stable version.
The `xserver` module links `/share/X11/xkb` to `/run/current-system` to
make this possible.
The fix can be tested inside the following VM:
```
{
zshtest = {
programs.zsh.enable = true;
users.extraUsers.vm = {
password = "vm";
isNormalUser = true;
};
services.xserver.enable = true;
};
}
```
Fixes NixOS#46025
Ma27
force-pushed
the
fix-setxkbmap-completion
branch
from
0f25d11
to
18d4615
Compare
|
we now use The |
|
Success on x86_64-linux (full log) Attempted: zsh Partial log (click to expand)
|
|
Success on aarch64-linux (full log) Attempted: zsh Partial log (click to expand)
|
|
as per #46764, you might want to add the patch to nixpkgs instead. btw, thanks for the diligent work on this issue :) |
|
I don't think that the issue applies here. This only happens when pulling a commit from a pull request which will be gone as soon as the branch is force-pushed. In this case the patch is already in |
|
right I got a bit hasty :shame: |
|
merging for now. This fixes an actual bug and the patch has been accepted by ZSH upstream, so in case of the next bump we may need to remove it, but this should work for now. |
|
thanks for your work !
Le ven. 19 oct. 2018 à 21:33, Maximilian Bosch <notifications@github.com> a
écrit :
… Merged #46152 <#46152> into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#46152 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA2FOlW1ihitGnxx0CKxBJ2VGHWJcsDHks5umcaMgaJpZM4Wc8hZ>
.
|
Instead of searching
/usrit should search for thexkbdirs theproper store path should be searched.
Fixes #46025
/cc @teto
Motivation for this change
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)