Bug 251987 - multimedia/get_iplayer: rc.d script emits error message even when disabled
Summary: multimedia/get_iplayer: rc.d script emits error message even when disabled
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jason E. Hale
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-20 13:54 UTC by Niels Bakker
Modified: 2021-02-02 14:23 UTC (History)
2 users (show)

See Also:
jamie: maintainer-feedback+
jhale: merge-quarterly+


Attachments
fix to rc.d script - bump revision number (1.13 KB, patch)
2020-12-22 13:05 UTC, Jamie Landeg-Jones
jamie: maintainer-approval+
Details | Diff
fix to rc.d script, fix typo, and bump revision number (1.52 KB, patch)
2020-12-22 20:14 UTC, Jamie Landeg-Jones
jamie: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Niels Bakker 2020-12-20 13:54:42 UTC
The latest version of get_iplayer (ports r541176) introduced the following code:

export HOME="$get_iplayer_chdir"
cd "$HOME" || exit 1

This is always executed irrespective of rcvar get_iplayer_enable. The default homedir for the get_iplayer user is /nonexistent. Therefore, upon each boot, the script would emit the following error:

cd: /nonexistent: No such file or directory

with no indication of the source of the error.
Comment 1 Jamie Landeg-Jones 2020-12-22 13:05:02 UTC
Created attachment 220812 [details]
fix to rc.d script - bump revision number

Ah, thanks for noticing that. A stupid mistake (merging a non-rc.d script into the rc.d script).

... and made worse by the fact that I reported a similar error in someone elses startup script a few months ago!

The applied patch fixes it, can someone commit please?

Cheers, Jamie
Comment 2 Niels Bakker 2020-12-22 13:48:36 UTC
Shouldn't '$get_iplayer_chdir' in the last line be ${get_iplayer_chdir} to allow for variable expansion?
Comment 3 Jamie Landeg-Jones 2020-12-22 18:10:05 UTC
(In reply to Niels Bakker from comment #2)

No, the braces aren't required for simple variable substitutions.

I note that RC scripts do tend to use them anyway, so for style reasons, the committer can add them if they want, but they aren't necessary.
Comment 4 Niels Bakker 2020-12-22 18:15:33 UTC
It was more about the single quotes:

$ start_precmd="export foo='$HOME'"
$ $start_precmd
$ echo $foo
'/home/niels'
$ pwd
/home/niels
$ cd "$foo"
cd: '/home/niels': No such file or directory
$ 

If rc does an eval $start_precmd instead of merely invoking it like above, this may not be needed and your version may work as expected (or as I didn't expect it to work).
Comment 5 Jamie Landeg-Jones 2020-12-22 20:12:11 UTC
(In reply to Niels Bakker from comment #4)

Ahh, ok. Yes, the whole thing is passed through "eval", and would otherwise fail without quotes if the home directory contained the space character, which is perfectly valid. So the patch works.

However, on further inspection, rc.subr itself isn't compatible with space in directories (xxx_chdir fails), so I guess this is moot.

Of course, the single quoting will still cause the script to fail if the path contains a "'", but this is opening up another can of worms.... The whole rc.subr system needs to be changed to cope with shell-special characters really.

However, I have updated the patch to use the proper way of exporting variables to daemons, and also fixed a typo in the original pkg-message.

Is this ok?

Cheers, Jamie
Comment 6 Jamie Landeg-Jones 2020-12-22 20:14:32 UTC
Created attachment 220825 [details]
fix to rc.d script, fix typo, and bump revision number
Comment 7 Niels Bakker 2020-12-22 20:16:33 UTC
Looks superb to me!
Comment 8 Jamie Landeg-Jones 2020-12-22 21:05:11 UTC
(In reply to Niels Bakker from comment #7)

Cheers! :-)
Comment 9 Jamie Landeg-Jones 2021-01-04 02:58:20 UTC
Does anyone know if I need to submit a seperate maintainer-request to get this actioned?

Cheers
Comment 10 commit-hook freebsd_committer freebsd_triage 2021-02-02 14:16:09 UTC
A commit references this bug:

Author: jhale
Date: Tue Feb  2 14:15:43 UTC 2021
New revision: 563809
URL: https://svnweb.freebsd.org/changeset/ports/563809

Log:
  multimedia/get_iplayer: Fix rc.d script and typo in pkg-message

  The latest version of get_iplayer (ports r541176) introduced the following code:

  export HOME="$get_iplayer_chdir"
  cd "$HOME" || exit 1

  This is always executed irrespective of rcvar get_iplayer_enable. The default
  homedir for the get_iplayer user is /nonexistent. Therefore, upon each boot,
  the script would emit the following error:

  cd: /nonexistent: No such file or directory

  with no indication of the source of the error.

  PR:		251987
  Submitted by:	Jamie Landeg-Jones <jamie@catflap.org> (maintainer)
  Reported by:	Niels Bakker <niels=freebsd@bakker.net>
  MFH:		2021Q1

Changes:
  head/multimedia/get_iplayer/Makefile
  head/multimedia/get_iplayer/files/get_iplayer.in
  head/multimedia/get_iplayer/pkg-message
Comment 11 commit-hook freebsd_committer freebsd_triage 2021-02-02 14:18:11 UTC
A commit references this bug:

Author: jhale
Date: Tue Feb  2 14:17:16 UTC 2021
New revision: 563810
URL: https://svnweb.freebsd.org/changeset/ports/563810

Log:
  MFH: r563809

  multimedia/get_iplayer: Fix rc.d script and typo in pkg-message

  The latest version of get_iplayer (ports r541176) introduced the following code:

  export HOME="$get_iplayer_chdir"
  cd "$HOME" || exit 1

  This is always executed irrespective of rcvar get_iplayer_enable. The default
  homedir for the get_iplayer user is /nonexistent. Therefore, upon each boot,
  the script would emit the following error:

  cd: /nonexistent: No such file or directory

  with no indication of the source of the error.

  PR:		251987
  Submitted by:	Jamie Landeg-Jones <jamie@catflap.org> (maintainer)
  Reported by:	Niels Bakker <niels=freebsd@bakker.net>

Changes:
_U  branches/2021Q1/
  branches/2021Q1/multimedia/get_iplayer/Makefile
  branches/2021Q1/multimedia/get_iplayer/files/get_iplayer.in
  branches/2021Q1/multimedia/get_iplayer/pkg-message