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

Fix ebusd service argument passing #309517

Merged
merged 2 commits into from
May 24, 2024
Merged

Conversation

bobrippling
Copy link
Contributor

Description of changes

This resolves a bug introduced in #308500 / #264893, where ebusd arguments were changed from --scanconfig=all to --scanconfig all with the use of cli.toGNUCommandLineShell.

The bug is an unfortunate result of ebusd's argument parsing - the arguments must be separated by equals signs.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@bobrippling bobrippling force-pushed the fix/ebusd-args branch 2 times, most recently from 6f759ef to 0178554 Compare May 6, 2024 09:50
@bobrippling
Copy link
Contributor Author

I'm not familiar with the nix language and suspect this fix may not be the best approach, open to suggestions.

@nathan-gs are you able to recommend anyone that might help with improving my fix?

@bobrippling bobrippling force-pushed the fix/ebusd-args branch 11 times, most recently from 3b6e3a8 to b9a463a Compare May 12, 2024 21:39
@bobrippling bobrippling marked this pull request as ready for review May 13, 2024 16:05
@bobrippling
Copy link
Contributor Author

@nathan-gs this is ready for review and fixes ebusd in nixos, happy to request a review from others if you're short on time :)

@nathan-gs
Copy link
Member

For me it looks good! However let's also have a review from some experts of cli.toGNUCommandLineShell, just to be sure (@Gabriella439 / @Profpatsch)?

lib/cli.nix Outdated Show resolved Hide resolved
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

# how to separate an option from its flag;
# by default, there is no separator, so option `-c` and value `5`
# would become ["-c" "5"].
# This is useful if the command requires equals, for example, `-c=5`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is currently not rendered in the reference manual. If you're interested in improving that, feel free to make a PR to move these into the doc comment above the function! Check out the part on 'function arguments' here to see the convention for that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tip - would you mind taking a look at #315820? Also, do you have a link to the reference doc? I can't find where cli.nix appears when I've been searching

@infinisil infinisil merged commit 6c42e87 into NixOS:master May 24, 2024
27 checks passed
@bobrippling bobrippling deleted the fix/ebusd-args branch May 30, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants