Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

herbstluftwm extraconfig dose not support bash functions #5213

Open
2 tasks done
tincotema opened this issue Apr 2, 2024 · 5 comments · May be fixed by #5566
Open
2 tasks done

herbstluftwm extraconfig dose not support bash functions #5213

tincotema opened this issue Apr 2, 2024 · 5 comments · May be fixed by #5566
Assignees
Labels
bug triage Issues or feature request that have not been triaged yet

Comments

@tincotema
Copy link

Are you following the right branch?

  • My Nixpkgs and Home Manager versions are in sync

Is there an existing issue for this?

  • I have searched the existing issues

Issue description

Hi, i am in the process of porting my existing herbstlfutwm config to home-manager/nixos using the home-manager herbstluftwm module.
currently the function for generating the final autostart bash script dose not allow for bash functions in the extraconfig string.
my question is, why did you implement it in this way?
this part at the beginning:

shopt -s expand_aliases

# shellcheck disable=SC2142
alias herbstclient='set -- "$@" ";"'
set -- 

and this part at the end:

${cfg.package}/bin/herbstclient chain ";" "$@"

don't allow bash scripting, removing them restores the expected behaviour of an normal hebstluftwm autoconfig file.

to me these parts seem redundant since the autoconfig file it self is already been executed.

i know that as mentioned in #2884, you can port the bash function into the let ... in section of the nix config, but since i use some of the functions in an second bash script for monitor handling, i dont want to maintain two versions of the same code.

my suggestion would be to add an option to disable this behaviour by omitting the above mentioned code blocks from the config generator.

best
tincotema

Maintainer CC

@olmokramer

System information

- system: `"x86_64-linux"`
 - host os: `Linux 6.7.10, NixOS, 24.05 (Uakari), 24.05pre-git`
 - multi-user?: `no`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.2`
 - nixpkgs: `/nix/store/7ay2dd0w41iqvrnkx19v4vf4f5kkxvc2-source`
@tincotema tincotema added bug triage Issues or feature request that have not been triaged yet labels Apr 2, 2024
@olmokramer
Copy link
Contributor

Hi @tincotema, sorry for the late reply. The point of that alias and only executing all commands at once with the herbstclient chain at the end is a performance improvement: instead of running the herbstclient program many times, it is called only once which speeds up the autostart script by a lot. Additionally you will see all changes being applied at once instead of one at a time.

But I can see how it can be annoying if you want to use bash functions in your autostart file. I'll create a PR to make this optional like you suggested.

@olmokramer olmokramer linked a pull request Jun 22, 2024 that will close this issue
6 tasks
Copy link

stale bot commented Sep 28, 2024

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

  • If this is resolved, please consider closing it so that the maintainers know not to focus on this.
  • If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

  • If you are also experiencing this issue, please add details of your situation to help with the debugging process.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

@stale stale bot added the status: stale label Sep 28, 2024
@olmokramer
Copy link
Contributor

Still relevant, there is a PR at #5566 but it hasn't been merged yet.

@stale stale bot removed the status: stale label Sep 30, 2024
Copy link

stale bot commented Jan 3, 2025

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

  • If this is resolved, please consider closing it so that the maintainers know not to focus on this.
  • If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

  • If you are also experiencing this issue, please add details of your situation to help with the debugging process.
  • If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

@stale stale bot added the status: stale label Jan 3, 2025
@olmokramer
Copy link
Contributor

Still relevant, previously mentioned PR is still good but hasn't been merged yet.

@stale stale bot removed the status: stale label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug triage Issues or feature request that have not been triaged yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants