FreeBSD: fix sed inplace usage, use bin/sh as shebang for script, pkg make director dependent of database-postgresql by bruno-at-bareos · Pull Request #1961 · bareos/bareos

bruno-at-bareos · 2024-09-18T14:45:35Z

fix issue FreeBSD of sed inplace FreeBSD avoid creating configuration files ending with .conf'' or .conf"" #1960
revert changes made on db script and use again /bin/sh as shebang
make bareos.com-director dependent of bareos.com-database-postgresql

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General

Is the PR title usable as CHANGELOG entry?
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Required backport PRs have been created
Correct milestone is set

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR

arogge

The change to the "am I running in Windows"-check is not proper. The rest looks great though.

arogge · 2024-09-27T10:23:41Z

core/src/cats/grant_bareos_privileges.in


 retval=0
-if [[ "$(uname -s)" =~ _NT ]]; then
+if [ "$(uname -s)" =~ _NT ]; then


ShellCheck says "In POSIX sh, =~ regex matching is undefined.", so chances are this will break sooner or later.
Maybe we could use command -v cygpath to check if cygpath is available instead?

ShellCheck says "In POSIX sh, =~ regex matching is undefined.", so chances are this will break sooner or later. Maybe we could use command -v cygpath to check if cygpath is available instead?

would that work also with the new native build and tests ?

I think so, yes.

After a quick check to decide if we want to go for bash require. Actual Next doesn't require bash under FreeBSD.

pkg install bareos.com-director bareos.com-bconsole bareos.com-filedaemon bareos.com-storage Updating FreeBSD repository catalogue... FreeBSD repository is up to date. Updating Bareos repository catalogue... pkg: Repository Bareos has a wrong packagesite, need to re-create database Fetching meta.conf: 100% 163 B 0.2kB/s 00:01 Fetching packagesite.pkg: 100% 3 KiB 2.9kB/s 00:01 Processing entries: 100% Bareos repository update completed. 12 packages processed. All repositories are up to date. The following 9 package(s) will be affected (of 0 checked): New packages to be INSTALLED: bareos.com-bconsole: 24.0.0.p1181.339.b13 [Bareos] bareos.com-common: 24.0.0.p1181.339.b13 [Bareos] bareos.com-database-common: 24.0.0.p1181.339.b13 [Bareos] bareos.com-database-tools: 24.0.0.p1181.339.b13 [Bareos] bareos.com-director: 24.0.0.p1181.339.b13 [Bareos] bareos.com-filedaemon: 24.0.0.p1181.339.b13 [Bareos] bareos.com-storage: 24.0.0.p1181.339.b13 [Bareos] jansson: 2.14 [FreeBSD] lzo2: 2.10_1 [FreeBSD] Number of packages to be installed: 9 The process will require 9 MiB more space. 2 MiB to be downloaded. Proceed with this action? [y/N]:

of course on a system that doesn't have bash installed :-)

@pstorz propose eventually to go that way
https://docs.freebsd.org/en/books/porters-handbook/uses/#uses-shebangfix

To remove the bashism, we could use the case statement for a substring match, like so:

case $(uname -s) in *_NT) i_am_running_in_windows=yes ;; *) i_am_running_in_windows=no ;; esac

So to resume, we decided to keep POSIX scripts for the database, so back to /bin/sh and we use the case xxx statement to test the Windows usage.

should be fixed correctly now with 2cba3da

arogge

I like the idea with the case, that's probably the nicest pattern matching we can have in a posix shell.
However, some of the other small changes you introduced, will not do what you expected.

arogge · 2024-10-02T14:43:17Z

core/src/cats/grant_bareos_privileges.in

-fi
+case $(uname -s) in
+  *_NT)
+    PAGER="" PGOPTIONS="--client-min-messages=warning" psql --no-psqlrc -f "$(cygpath -w \"${temp_sql_grants}\")" -d "${db_name}" || retval=$?


Adding the backslashes here is actually harmful: the shell parser treats the contents of $() as a completely separate context with separated quoting.

Thus, echo "$(echo "x y")" will emit x y while echo "$(echo \"x y\")" will emit "x y".
There are not only quotes around the expression, but the second space in the middle was lost. This is because echo gets called with $1 being "x and $2 being y".

Suggested change

PAGER="" PGOPTIONS="--client-min-messages=warning" psql --no-psqlrc -f "$(cygpath -w \"${temp_sql_grants}\")" -d "${db_name}" || retval=$?

PAGER="" PGOPTIONS="--client-min-messages=warning" psql --no-psqlrc -f "$(cygpath -w "${temp_sql_grants}")" -d "${db_name}" || retval=$?

Ok I will complain to shellcheck maintainer :-)

arogge · 2024-10-02T14:54:06Z

core/src/cats/grant_bareos_privileges.in

+case $(uname -s) in
+  *_NT)
+    PAGER="" PGOPTIONS="--client-min-messages=warning" psql --no-psqlrc -f "$(cygpath -w \"${temp_sql_grants}\")" -d "${db_name}" || retval=$?
+    rm -f "$(cygpath -w \"${temp_sql_schema}\")"


this will not work.
rm needs the cygwin-path (i.e. /cygdrive/c/...) and not the translated windows path (i.e. C:\...).

That's what you get to try to optimize it too far :-) I will revert this changes.

done in 2cba3da

arogge

Looks good to me!

We remove the refactoring, as the expression set in variable will created artifact file with the delimiter like file'' or file"". Add a comment in code to explain the risk of refactoring that part. Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>

Reverted changes made in #2204efeef4 for `make_bareos_tables`, `grant_bareos_privileges` and `update_bareos_tables`. We adapt the test detecting Windows to stay posix compatible, which fix FreeBSD catalog management. Sanitize by proper double-quoting variables. Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>

- add `#include <signal.h>` for build success on FreeBSD

bruno-at-bareos added this to the 24.0.0 milestone Sep 18, 2024

bruno-at-bareos force-pushed the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch from 99432e7 to 422cc8e Compare September 19, 2024 08:18

arogge added requires backport to 23 bug This addresses a bug labels Sep 19, 2024

arogge assigned bruno-at-bareos Sep 19, 2024

arogge self-requested a review September 24, 2024 09:48

bruno-at-bareos force-pushed the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch 6 times, most recently from 5a182cb to 091d00b Compare September 25, 2024 09:16

bruno-at-bareos assigned arogge Sep 25, 2024

bruno-at-bareos changed the title ~~fix sed inplace usage on bsd like OS~~ FreeBSD: fix sed inplace usage, use bin/sh as shebang for script, pkg make director dependent of database-postgresql Sep 25, 2024

bruno-at-bareos force-pushed the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch from 091d00b to 801b37b Compare September 25, 2024 11:35

bruno-at-bareos mentioned this pull request Sep 25, 2024

FreeBSD: fix sed inplace usage, pkg make director dependent of database-postgresql #1964

Merged

7 tasks

bruno-at-bareos linked an issue Sep 26, 2024 that may be closed by this pull request

FreeBSD avoid creating configuration files ending with .conf'' or .conf"" #1960

Closed

bruno-at-bareos requested a review from pstorz September 26, 2024 09:39

bruno-at-bareos assigned pstorz Sep 26, 2024

arogge requested changes Sep 27, 2024

View reviewed changes

bruno-at-bareos force-pushed the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch 3 times, most recently from 137e2ac to 283c790 Compare October 1, 2024 15:19

bruno-at-bareos requested a review from arogge October 2, 2024 09:26

arogge requested changes Oct 2, 2024

View reviewed changes

bruno-at-bareos force-pushed the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch from 283c790 to 2cba3da Compare October 4, 2024 12:27

bruno-at-bareos requested a review from arogge October 4, 2024 12:28

bruno-at-bareos force-pushed the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch from afcb458 to 712acfb Compare October 9, 2024 08:43

arogge approved these changes Oct 9, 2024

View reviewed changes

bruno-at-bareos and others added 4 commits October 9, 2024 15:13

freebsd: make director depend on database-postgresql

6d9c0a0

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>

tests: fix SIGUSR2 and SIGPIPE initialization

06b0f92

- add `#include <signal.h>` for build success on FreeBSD

Update CHANGELOG.md

b57d131

BareosBot force-pushed the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch from 4c41266 to b57d131 Compare October 9, 2024 15:14

BareosBot merged commit 2939326 into bareos:master Oct 9, 2024

bruno-at-bareos deleted the dev/bruno/master/fix-1960-bsdlike-sed-in-place branch November 25, 2024 10:27

	PAGER="" PGOPTIONS="--client-min-messages=warning" psql --no-psqlrc -f "$(cygpath -w \"${temp_sql_grants}\")" -d "${db_name}" \|\| retval=$?
	PAGER="" PGOPTIONS="--client-min-messages=warning" psql --no-psqlrc -f "$(cygpath -w "${temp_sql_grants}")" -d "${db_name}" \|\| retval=$?

Conversation

bruno-at-bareos commented Sep 18, 2024 • edited by arogge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bruno-at-bareos Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bruno-at-bareos commented Sep 18, 2024 •

edited by arogge

Loading

bruno-at-bareos Sep 30, 2024 •

edited

Loading