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

build: fix some shellcheck issues & use config.sh in more scripts #5927

Merged
merged 2 commits into from
Jul 30, 2023

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented Jul 28, 2023

This removes the need to manually pass variables such as $(TARNAME)
and $(VERSION) to shell scripts in the root Makefile.

Relates to #5140.

kmk3 added 2 commits July 28, 2023 14:08
This removes the need to manually pass variables such as `$(TARNAME)`
and `$(VERSION)` to shell scripts in the root Makefile.

Relates to netblue30#5140.
@kmk3 kmk3 requested a review from rusty-snake July 28, 2023 17:14
platform/rpm/mkrpm.sh Show resolved Hide resolved
@kmk3
Copy link
Collaborator Author

kmk3 commented Jul 28, 2023

@rusty-snake on Jul 28:

Just wondering what's the benefit of printf over plain echo here is?

Escape sequences (and certain flags) may be treated differently by different
echo implementations, so printf is better for portability, especially if
external input is involved. In this specific case it's likely not an issue,
but it's simpler to just use printf basically everywhere rather than having
to check each echo invocation for potential issues.

From echo(1p):

OPERANDS
       The following operands shall be supported:

       string    A string to be written to standard output. If the first
                 operand is -n, or if any of the operands contain a
                 <backslash> character, the results are implementation-
                 defined.
[...]

APPLICATION USAGE
       It is not possible to use echo portably across all POSIX systems unless
       both -n (as the first argument) and escape sequences are omitted.

       The printf utility can be used portably to emulate any of the
       traditional behaviors of the echo utility as follows (assuming that IFS
       has its standard value or is unset):
[...]

RATIONALE
       The echo utility has not been made obsolescent because of its extremely
       widespread use in historical applications. Conforming applications that
       wish to do prompting without <newline> characters or that could
       possibly be expecting to echo a -n, should use the printf utility
       derived from the Ninth Edition system.

       As specified, echo writes its arguments in the simplest of ways. The
       two different historical versions of echo vary in fatally incompatible
       ways.

       The BSD echo checks the first argument for the string -n which causes
       it to suppress the <newline> that would otherwise follow the final
       argument in the output.

       The System V echo does not support any options, but allows escape
       sequences within its operands, as described for XSI implementations in
       the OPERANDS section.

       The echo utility does not support Utility Syntax Guideline 10 because
       historical applications depend on echo to echo all of its arguments,
       except for the -n option in the BSD version.

@rusty-snake
Copy link
Collaborator

Learned, Thanks.

Have to say that a #!/bin/bash script for a program that fundamentally relies on Linux API will hardly never see an other implementation.

@kmk3
Copy link
Collaborator Author

kmk3 commented Jul 28, 2023

@rusty-snake on Jul 28:

Learned, Thanks.

No problem.

Have to say that a #!/bin/bash script for a program that fundamentally
relies on Linux API will hardly never see an other implementation.

Note that even in bash, with echo you need to make sure that the arguments do
not start with - nor contain escape sequences, while with printf you can
just use one or more %s in the format string and put the variables afterwards
(which is arguably easier to check for correctness in most cases).

It also helps with consistency with /bin/sh scripts, which may point to a
shell with an incompatible echo implementation.

I'm not sure what is the relation of "a #!/bin/bash script" and "a program
that fundamentally relies on Linux API" (if you're referring to firejail), as
firejail can be used in distributions where bash is not the default shell (such
as Alpine) and considering that the script in question does not make
significant use of bash-specific features.

@kmk3 kmk3 merged commit 4a26d91 into netblue30:master Jul 30, 2023
@kmk3 kmk3 deleted the build-use-config-sh branch July 30, 2023 00:54
kmk3 added a commit that referenced this pull request Jul 30, 2023
kmk3 added a commit that referenced this pull request Aug 12, 2023
This fixes the following errors:

    $ make clean
    [...]
    cd test/compile; ./compile.sh --clean; cd ../..
    ./compile.sh: line 55: TARNAME: command not found
    ./compile.sh: line 55: VERSION: command not found

This amends commit 200f389 ("build: use config.sh in more scripts",
2023-07-28) / PR #5927.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

2 participants