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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

systemd: Simplify unit script names #85428

Merged
merged 4 commits into from May 12, 2020

Conversation

@kirelagin
Copy link
Member

kirelagin commented Apr 17, 2020

Make journalctl logs readable again!

  • Remove the superfluous unit-script prefix. Personally I can tell it鈥檚 a script by its distinctive -start / -preStart / etc. suffix.
  • Put it back into /bin/ (adba92b 馃槖) because I don鈥檛 want to see those hashes in my journalctl output (this and some services printing their own timestamps makes the actual log message start on 3/4 of my screen, and I am not exaggerating).

cc @lheckemann

Current journal output from services started by `script` rather than
`ExexStart` is unreadable because the name of the file (which journalctl
records and outputs) quite literally takes 1/3 of the screen (on smaller
screens).

Make it shorter. In particular:

* Drop the `unit-script` prefix as it is not very useful.
* Use `writeShellScriptBin` to write them because:
  * It has a `checkPhase` which is better than no checkPhase.
  * The script itself ends up having a short name.
@kirelagin
Copy link
Member Author

kirelagin commented Apr 17, 2020

Oh, I just noticed that @lheckemann added this unix-script prefix in the same commit. Are we going to fight? 馃
I suppose, I can concede either unit- or script-, but not both. (And to be honest, I still don鈥檛 see the need in either of those.)

@lheckemann
Copy link
Member

lheckemann commented Apr 17, 2020

see discussion at #43534

in pkgs.writeTextFile { name = mkScriptName name; executable = true; inherit text; };
let
scriptName = replaceChars [ "\\" "@" ] [ "-" "_" ] (shellEscape name);
out = pkgs.writeShellScriptBin scriptName ''

This comment has been minimized.

Copy link
@zhenyavinogradov

zhenyavinogradov Apr 17, 2020

writeShellScriptBin will include the shebang, so you can remove it from makeJobScript invocations below

This comment has been minimized.

Copy link
@kirelagin

kirelagin Apr 17, 2020

Author Member

Oh, yes, I meant to remove it, thanks.

@kirelagin
Copy link
Member Author

kirelagin commented Apr 17, 2020

Looking at the discussion, it seems that people just like seeing unit-script in their /nix/store paths, is that right? In this case a solution I can propose is to use writeTextFile and add unit-script in the derivation name, but not in the script file name.

(However, personally I would prefer not to do this to keep things simple.)

cc @fpletz @globin @grahamc

kirelagin added 2 commits Apr 17, 2020
Add a distinctive `unit-script` prefix to systemd unit scripts to make
them easier to find in the store directory. Do not add this prefix to
actual script file name as it clutters logs.
@kirelagin
Copy link
Member Author

kirelagin commented Apr 17, 2020

If people insist on keeping the prefix, here is a commit that implements this ^. However, as I said, I would really prefer to drop it.

* Avoid extra string interpolation.
@kirelagin kirelagin force-pushed the serokell:kirelagin/unit-script-name branch from 8d39c3e to daac85d Apr 20, 2020
@kirelagin
Copy link
Member Author

kirelagin commented Apr 23, 2020

ping

@kirelagin
Copy link
Member Author

kirelagin commented May 7, 2020

馃憢

@lheckemann
Copy link
Member

lheckemann commented May 8, 2020

@ofborg test boot

@lheckemann
Copy link
Member

lheckemann commented May 8, 2020

I'd merge this if the tests pass and nobody voices any objections until then.

@lheckemann lheckemann merged commit 90c0191 into NixOS:master May 12, 2020
18 checks passed
18 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="daac85d"; rev="daac85d9916ea095eadf5c89d685dd9057c8d48f"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="daac85d"; rev="daac85d9916ea095eadf5c89d685dd9057c8d48f"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="daac85d"; rev="daac85d9916ea095eadf5c89d685dd9057c8d48f"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="daac85d"; rev="daac85d9916ea095eadf5c89d685dd9057c8d48f"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="daac85d"; rev="daac85d9916ea095eadf5c89d685dd9057c8d48f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="daac85d"; rev="daac85d9916ea095eadf5c89d685dd9057c8d48f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="daac85d"; rev="daac85d9916ea095eadf5c89d685dd9057c8d48f"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
systemd, systemd.passthru.tests on aarch64-linux Success
Details
systemd, systemd.passthru.tests on x86_64-linux Success
Details
tests.boot on aarch64-linux Success
Details
tests.boot on x86_64-linux Success
Details
@bjornfor
Copy link
Contributor

bjornfor commented May 12, 2020

Thanks for fixing this!

@kirelagin
Copy link
Member Author

kirelagin commented May 12, 2020

Thanks! If you liked this change, please, help me get #87219 merged too :).

@datafoo
Copy link
Contributor

datafoo commented May 22, 2020

Will these changes be back-ported to nixos-20.03-small and nixos-20.03 channels?

@datafoo
Copy link
Contributor

datafoo commented May 27, 2020

Sorry if that sounds stupid. I haven't fully understand how/when changes committed on the master branch of this repo end up in a channel.
I was interested to know if these changes will be visible here: https://github.com/NixOS/nixpkgs-channels/blob/nixos-20.03-small/nixos/modules/system/boot/systemd.nix

@Mic92
Copy link
Contributor

Mic92 commented May 27, 2020

We usually backport bugfixes and keep major changes in master. I don't know in which category this change falls. If the maintainer don't want to backport it, it will be available in 20.09.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can鈥檛 perform that action at this time.