Fix shelltime service not running on Linux#168
Conversation
The Linux systemd service was failing to start due to two issues:
1. BaseFolder was missing from template data in GetDaemonServiceFile,
causing {{.BaseFolder}} to render as empty, resulting in invalid
daemon path "/bin/shelltime-daemon" instead of the correct path.
2. $USER environment variable is not available in systemd user services,
causing getent passwd $USER to fail. Replaced with $(id -un) which
reliably returns the current username without depending on environment.
This aligns the Linux implementation with the working macOS version.
Summary of ChangesHello @AnnatarHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves two critical issues that prevented the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses two issues that were preventing the shelltime systemd service from starting on Linux. The fixes, which involve adding a missing BaseFolder to the template data and using a more reliable method to get the username, are appropriate and directly solve the problems described.
I've included a couple of suggestions for further improvement. The main suggestion is to refactor the ExecStart command in the systemd service file to use systemd's native specifiers (%s and %h). This would make the service definition cleaner, more robust, and independent of shell command parsing, while also simplifying the Go code that generates the service file.
| err = tmpl.Execute(&buf, map[string]string{ | ||
| "UserName": username, | ||
| "UserName": username, | ||
| "BaseFolder": l.baseFolder, |
There was a problem hiding this comment.
This line correctly adds the BaseFolder to the template data, fixing one of the bugs.
However, as mentioned in my comment on model/sys-desc/shelltime.service, the service file can be improved to use systemd's %h specifier for the home directory. If you apply that suggestion, {{.BaseFolder}} will no longer be needed in the template, and this line can be removed.
| [Service] | ||
| Type=simple | ||
| ExecStart=/bin/sh -c 'exec $(getent passwd $USER | cut -d: -f7) -l -c "{{.BaseFolder}}/bin/shelltime-daemon"' | ||
| ExecStart=/bin/sh -c 'exec $(getent passwd $(id -un) | cut -d: -f7) -l -c "{{.BaseFolder}}/bin/shelltime-daemon"' |
There was a problem hiding this comment.
While this change correctly fixes the issue with $USER not being available, the ExecStart command can be significantly simplified and made more robust by using systemd's built-in specifiers.
You can replace the complex shell invocation with a more direct command that uses %s for the user's shell and %h for the user's home directory. This avoids shelling out to id, getent, and cut, making the service file cleaner and less dependent on external commands. Adopting this would also make the change in daemon-installer.linux.go unnecessary.
ExecStart=%s -l -c "%h/.shelltime/bin/shelltime-daemon"
2 participants
The Linux systemd service was failing to start due to two issues:
BaseFolder was missing from template data in GetDaemonServiceFile, causing {{.BaseFolder}} to render as empty, resulting in invalid daemon path "/bin/shelltime-daemon" instead of the correct path.
$USER environment variable is not available in systemd user services, causing getent passwd$USER to fail. Replaced with $ (id -un) which reliably returns the current username without depending on environment.
This aligns the Linux implementation with the working macOS version.