unitd 1.30.0 change the option --tmp to --tmpdir

[elrido]

Hi there and thanks for this cool project. It seems that the latest release 1.30.0 changed the option --tmp to --tmpdir, so when starting the daemon with the same arguments as with 1.29.1, it errors out with:

unknown option "--tmp", try "/usr/sbin/unitd -h" for available options

We found this in a smoketest on an alpine based image that makes use of it - alpine upgraded to 1.30.0 shortly after releasing their stable release (3.18).

I would suggest that we document this change in behaviour for 1.30.0 - I can raise a PR against the CHANGES file, if that is Ok?

I'm not sure if this is a fully intentional change, though. It looks to me as if this was changed in 5a37171#diff-bfe6a52baf895e8f15c2c57cc0e7365440c73301ea0597acc9ba7bb88aa9ee7fR1068 and the commit message reads to me as if this was intended to deprecate tmp for tmpdir, but keep the old options supported, for now.

[tippexs]

Thanks @elrido for letting us know. This is indeed an blind spot in our CHANGES. No PR needed. We will take care of it. Sorry you had to find this out the hard way. We will do better the next time 😇

[alejandro-colomar]

@elrido wrote:

Hi there and thanks for this cool project. It seems that the latest release 1.30.0
changed the option --tmp to --tmpdir , so when starting the daemon with the same
arguments as with 1.29.1, it errors out with:

unknown option "--tmp", try "/usr/sbin/unitd -h" for available options

We found this in a smoketest on an alpine based image
PrivateBin/docker-unit-alpine#2 that makes use of it -
alpine upgraded to 1.30.0 shortly after releasing their stable release (3.18).

I would suggest that we document this change in behaviour for 1.30.0 - I can raise
a PR against the CHANGES

unit/CHANGES

Line 2 in aa45d17

Changes with Unit 1.30.0 10 May 2023

file, if that is Ok?

I'm not sure if this is a fully intentional change, though. It looks to me as if
this was changed in
5a37171#diff-
bfe6a52baf895e8f15c2c57cc0e7365440c73301ea0597acc9ba7bb88aa9ee7fR1068 and the
commit message reads to me as if this was intended to deprecate tmp for tmpdir,
but keep the old options supported, for now.

You're right, it was changed in that patch; and it wasn't intentional;
as you say, the intention was to keep the deprecated options for some
time. I kept deprecated options for the configure script, but forgot
about the unitd(8) binary.

Below is a patch that fixes this, and prints appropriate deprecation
warnings.

@tippexs wrote:

Thanks @elrido for letting us know. This is indeed an blind spot in our CHANGES.
No PR needed. We will take care of it. Sorry you had to find this out the hard
way. We will do better the next time 😇

My bad. I think there's no need to change the Changes, since it wasn't
an intended break. Rather, it was a bug in the patch, and we could
release a 1.30.1 with this bugfix, and of course a Changes entry
mentioning the fix.

Here's the fix:

diff --git a/src/nxt_runtime.c b/src/nxt_runtime.c
index 96f801fb..2bc608ef 100644
--- a/src/nxt_runtime.c
+++ b/src/nxt_runtime.c
@@ -966,6 +966,10 @@ nxt_runtime_conf_read_cmd(nxt_task_t *task, nxt_runtime_t *rt)
                        "option \"--statedir\" requires directory\n";
     static const char  no_tmp[] = "option \"--tmpdir\" requires directory\n";
 
+    static const char  modules_deprecated[] = "option \"--modules\" is deprecated; use \"--modulesdir\" instead\n";
+    static const char  state_deprecated[] = "option \"--state\" is deprecated; use \"--statedir\" instead\n";
+    static const char  tmp_deprecated[] = "option \"--tmp\" is deprecated; use \"--tmpdir\" instead\n";
+
     static const char  help[] =
         "\n"
         "unit options:\n"
@@ -992,6 +996,10 @@ nxt_runtime_conf_read_cmd(nxt_task_t *task, nxt_runtime_t *rt)
         "  --tmpdir DIR         set tmp directory name\n"
         "                       default: \"" NXT_TMPDIR "\"\n"
         "\n"
+        "  --modules DIR        [deprecated] synonym for --modulesdir\n"
+        "  --state DIR          [deprecated] synonym for --statedir\n"
+        "  --tmp DIR            [deprecated] synonym for --tmpdir\n"
+        "\n"
         "  --user USER          set non-privileged processes to run"
                                 " as specified user\n"
         "                       default: \"" NXT_USER "\"\n"
@@ -1073,7 +1081,14 @@ nxt_runtime_conf_read_cmd(nxt_task_t *task, nxt_runtime_t *rt)
             continue;
         }
 
+        if (nxt_strcmp(p, "--modules") == 0) {
+            write(STDERR_FILENO, modules_deprecated,
+                  nxt_length(modules_deprecated));
+            goto modulesdir;
+        }
+
         if (nxt_strcmp(p, "--modulesdir") == 0) {
+modulesdir:
             if (*argv == NULL) {
                 write(STDERR_FILENO, no_modules, nxt_length(no_modules));
                 return NXT_ERROR;
@@ -1086,7 +1101,14 @@ nxt_runtime_conf_read_cmd(nxt_task_t *task, nxt_runtime_t *rt)
             continue;
         }
 
+        if (nxt_strcmp(p, "--state") == 0) {
+            write(STDERR_FILENO, state_deprecated,
+                  nxt_length(state_deprecated));
+            goto statedir;
+        }
+
         if (nxt_strcmp(p, "--statedir") == 0) {
+statedir:
             if (*argv == NULL) {
                 write(STDERR_FILENO, no_state, nxt_length(no_state));
                 return NXT_ERROR;
@@ -1099,7 +1121,13 @@ nxt_runtime_conf_read_cmd(nxt_task_t *task, nxt_runtime_t *rt)
             continue;
         }
 
+        if (nxt_strcmp(p, "--tmp") == 0) {
+            write(STDERR_FILENO, tmp_deprecated, nxt_length(tmp_deprecated));
+            goto tmpdir;
+        }
+
         if (nxt_strcmp(p, "--tmpdir") == 0) {
+tmpdir:
             if (*argv == NULL) {
                 write(STDERR_FILENO, no_tmp, nxt_length(no_tmp));
                 return NXT_ERROR;

Here's some testing:

$ sudo unitd --modules foo
option "--modules" is deprecated; use "--modulesdir" instead
2023/05/16 23:53:17 [info] 26148#26148 unit 1.30.0 started
$ sudo killall unitd
$ sudo unitd --modules foo --state bar --tmp baz
option "--modules" is deprecated; use "--modulesdir" instead
option "--state" is deprecated; use "--statedir" instead
option "--tmp" is deprecated; use "--tmpdir" instead
2023/05/16 23:53:32 [alert] 26162#26162 Unable to create certificates storage directory: mkdir(bar/certs/) failed (2: No such file or directory)
2023/05/16 23:53:32 [alert] 26162#26162 Unable to create scripts storage directory: mkdir(bar/scripts/) failed (2: No such file or directory)
2023/05/16 23:53:32 [info] 26162#26162 unit 1.30.0 started
$ sudo killall unitd
$ sudo unitd --help

unit options:

  --version            print unit version and configure options

  --no-daemon          run unit in non-daemon mode

  --control ADDRESS    set address of control API socket
                       default: "unix:/usr/local/var/run/unit/control.unit.sock"

  --pid FILE           set pid filename
                       default: "/usr/local/var/run/unit/unit.pid"

  --log FILE           set log filename
                       default: "/usr/local/var/log/unit/unit.log"

  --modulesdir DIR     set modules directory name
                       default: "/usr/local/lib/unit/modules"

  --statedir DIR       set state directory name
                       default: "/usr/local/var/lib/unit"

  --tmpdir DIR         set tmp directory name
                       default: "/tmp"

  --modules DIR        [deprecated] synonym for --modulesdir
  --state DIR          [deprecated] synonym for --statedir
  --tmp DIR            [deprecated] synonym for --tmpdir

  --user USER          set non-privileged processes to run as specified user
                       default: "nobody"

  --group GROUP        set non-privileged processes to run as specified group
                       default: user's primary group

I didn't yet write the Changes entry, since we need to first create the
boilerplate.

Thanks!
Alex