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

lint: fix shellcheck for misc/systemv/nix-daemon #11103

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

fzakaria
Copy link
Contributor

Motivation

Got shellcheck passing for misc/systemv/nix-daemon

Not sure how to test this since it's not running on my NixOS machine and I see no references to it in the directory otherwise.

Context

See #10795

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Got shellcheck passing for misc/systemv/nix-daemon

Not sure how to test this since it's not running on my NixOS machine and
I see no references to it in the directory otherwise.

See NixOS#10795
@fzakaria fzakaria requested a review from edolstra as a code owner July 15, 2024 02:56
@@ -324,6 +324,7 @@
++ pkgs.nixComponents.nix-external-api-docs.nativeBuildInputs
++ [
pkgs.buildPackages.cmake
pkgs.shellcheck
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to run shellcheck myself manually.

Shellcheck has a diff output style that you can try to apply for auto fixes for lots of cases now
shellcheck misc/systemv/nix-daemon -f diff | git apply

. /etc/init.d/functions

LOCKFILE=/var/lock/subsys/nix-daemon
RUNDIR=/var/run/nix
PIDFILE=${RUNDIR}/nix-daemon.pid
RETVAL=0

base=${0##*/}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

warning from shellcheck that this has no reference.

@roberth
Copy link
Member

roberth commented Jul 16, 2024

Not sure how to test this since it's not running on my NixOS machine and I see no references to it in the directory otherwise.

Oof. If we don't have the tests to maintain this file, maybe we shouldn't pretend to.

Or would this be used in, say, NixBSD? Do they have a test infra we could reuse?
EDIT: asked them

@fzakaria
Copy link
Contributor Author

Cool -- in the meantime i'll pickup a different script.
We can merge when we hear back the answer.

@roberth
Copy link
Member

roberth commented Jul 16, 2024

Not used there.
I'm not sure how to proceed with this. It could be an improvement, but making a change without testing it also poses a risk.
Should we remove it?

@fzakaria
Copy link
Contributor Author

Not used there. I'm not sure how to proceed with this. It could be an improvement, but making a change without testing it also poses a risk. Should we remove it?

It's from 8 years ago with the description that it's for BSD.
if the BSD fork doesn't need it; I say now delete it.

@edolstra edolstra merged commit 6867cb1 into NixOS:master Jul 17, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants