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

Remove sh -c #1517

Closed
wants to merge 3 commits into from
Closed

Remove sh -c #1517

wants to merge 3 commits into from

Conversation

gilgongo
Copy link
Member

@@ -8,7 +8,7 @@ Type=simple
Restart=always
RestartSec=1
User=jamulus
ExecStart=/bin/sh -c '/usr/bin/jamulus -s -n -l /var/log/jamulus -e jamulus.fischvolk.de -g -o "$(uname -n);;"'
ExecStart=/usr/bin/jamulus -s -n -l /var/log/jamulus -e anygenre1.jamulus.io:22124 -g -o "$(uname -n);;"
Copy link
Member

@hoffie hoffie Apr 14, 2021

Choose a reason for hiding this comment

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

I suspect $(uname -n) will not expand properly without the sh -c wrap. This would likely need the exec as @softins suggested (haven't tried myself):

Suggested change
ExecStart=/usr/bin/jamulus -s -n -l /var/log/jamulus -e anygenre1.jamulus.io:22124 -g -o "$(uname -n);;"
ExecStart=/bin/sh -c 'exec /usr/bin/jamulus -s -n -l /var/log/jamulus -e anygenre1.jamulus.io:22124 -g -o "$(uname -n);;"'

Copy link
Member Author

Choose a reason for hiding this comment

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

I would expect it to be OK since it's not a sh only thing. Putting sh -c back here will cause the problem with SIGUSR2, no?

Choose a reason for hiding this comment

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

I wasn't aware of the second file. @hoffie suggested change will work. I already tested it with SIGUSR2.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I read https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Specifiers correctly

ExecStart=/usr/bin/jamulus -s -n -l /var/log/jamulus -e anygenre1.jamulus.io:22124 -g -o %H

would work.

Copy link

@helgeerbe helgeerbe left a comment

Choose a reason for hiding this comment

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

This looks good for me. And will be consistent with the wiki documentation.

Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

I feel strongly that the correct solution is to retain the sh -c and add the exec.

@gilgongo
Copy link
Member Author

@softins I see you're going to do that so I'll close this.

@gilgongo gilgongo closed this Apr 14, 2021
@pljones pljones deleted the gilgongo-sh branch September 5, 2021 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants