OS400/makefile.sh: fix shellcheck warning SC2038

[vszakats]

Also:

• OS400/makefile.sh: use end-of-options marker in xargs command.
• OS400/make-tests.sh: drop warning suppression.
  Seems to not trigger anymore as of shellcheck 0.11.0

[monnerat]

This breaks OS400 builds: neither -print0 nor xargs -0 supported on OS400.

[vszakats]

Is there a reason to use ls -S (sort by size) in this case? i.e. would this work to avoid the warning?:

find "${TOPDIR}" -type f | while read -r CCSID FILE

[monnerat]

Is there a reason to use ls -S ...

Yes: the -S option causes the file's encoding (CCSID) to be displayed. as this sequence makes sure files are UTF8-encoded, this is mandatory.

You should never assume qshell commands are GNU-compatible: POSIX is generally a good subset.

[monnerat]

IMHO, you better should exclude completely qshell scripts from your checks.

[vszakats]

I don't think making certain files "off-limits" is a solution.

[vszakats]

Going with this, -print is optional as in POSIX, -- is supported according to Qshell PDF:

find "${TOPDIR}" -type f | xargs ls -S -- | while read -r CCSID FILE

FWIW I can see no GNU things here, and in POSIX ls -S has a different meaning:

Sort with the primary key being file size (in decreasing order) and the secondary key being filename in the collating sequence (in increasing order).

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html

find: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/find.html

Qshell PDF here: https://www.ibm.com/docs/en/ssw_ibm_i_74/pdf/rzahzpdf.pdf

[vszakats]

If it's in the repository it cannot be off-limits IMO.

[monnerat]

find "${TOPDIR}" -type f | xargs ls -S -- | while read -r CCSID FILE

Yes, this should work. The -print and -- have never be the problem.

If it's in the repository it cannot be off-limits IMO.

What do you mean by "off-limits"?
There are plenty of "foreign" command syntaxes under the packages directory, i.e.: vms/*.com.
If the .sh extension confuses you, I can rename those files.

[vszakats]

No, .sh doesn't confuse me. What confuses everyone (incl. tools) is that these shell scripts have the #!/bin/sh POSIX shell shebang, while apparently they have nothing to do with the POSIX standard.

[vszakats]

It's not a very good experience when one hears first of "qshell" while being scolded for trying to maintain and as a consequence blindly fix what looks like a standard script!

edit: qshell has a single mention at the end of the OS/400 readme, in a way that's hard to guess it's related to .sh scripts (and impossible if someone doesn't know what to look for!). And 2 hits when searching in past issues, this being the 3rd one. Perhaps this could use a bit more emphasis, I linked to the documentation in the latest commit, for the next person trying to figure this out from nothing.

[monnerat]

What confuses everyone (incl. tools) is that these shell scripts have the #!/bin/sh POSIX shell shebang, while apparently they have nothing to do with the POSIX standard.

Let me see if we can suppress the shebang. I'll come back later on this.

It's not a very good experience when one hears first of "qshell" while being scolded for trying to maintain and as a consequence blindly fix what looks like a standard script!.

Sure., and I don't blame you but rather thank you to keep maintaining these files according to your views and changes elsewhere.

qshell has a single mention at the end of the OS/400 readme.

This document is primarily oriented towards OS/400 people wanting to use/compile/develop with curl on their favorite platform. I did not think of OS/400-unaware people when I wrote it. Maybe I should write some other document for curl developers?

[monnerat]

Let me see if we can suppress the shebang. I'll come back later on this.

I just performed a build without the shebangs successfully. Would you like I drop them ?

[vszakats]

What confuses everyone (incl. tools) is that these shell scripts have the #!/bin/sh POSIX shell shebang, while apparently they have nothing to do with the POSIX standard.

Let me see if we can suppress the shebang. I'll come back later on this.

It's not a very good experience when one hears first of "qshell" while being scolded for trying to maintain and as a consequence blindly fix what looks like a standard script!.

Sure., and I don't blame you but rather thank you to keep maintaining these files according to your views and changes elsewhere.

qshell has a single mention at the end of the OS/400 readme.

This document is primarily oriented towards OS/400 people wanting to use/compile/develop with curl on their favorite platform. I did not think of OS/400-unaware people when I wrote it. Maybe I should write some other document for curl developers?

I think it's enough if the scripts have e.g. a comment saying what
they need to comply with, along with an URL/pointer to the best
place to find its documentation.

Since by and large they are POSIX-compliant, the shebang can
remain IMO. It's useful if shellcheck runs through them too, and
can keep pass.

Perhaps having fewer scripts by merging them could help, and
unrelated but I think it'd be useful to sync the indenting with
the rest of scripts to ease maintenance. (I'm happy to do it)

[vszakats]

Let me see if we can suppress the shebang. I'll come back later on this.

I just performed a build without the shebangs successfully. Would you like I drop them ?

Ref my other comment, I think it's better to have some shebang, and
perhaps even better to have the existing one, so that shellcheck keeps
an eye on them. They rarely cause an issue in practice.

For this particular issue it'd be nice to find something that passes this
warning (and the issue behind it, which is a real one), but no idea what
could that be in qshell.

[monnerat]

... and the issue behind it, which is a real one ...

If I correctly understand the "issue": the original command incorrectly deals with names containing spaces and those starting with an hyphen. Right?
But we don't have any of those in curl tree! I wish and hope we would never have!

[vszakats]

... and the issue behind it, which is a real one ...

If I correctly understand the "issue": the original command incorrectly deals with names containing spaces and those starting with an hyphen. Right? But we don't have any of those in curl tree! I wish and hope we would never have!

Also quotes in filenames. Here's what shellcheck says about it:
https://www.shellcheck.net/wiki/SC2038

They are not in the curl repo, but there is no guarantee there aren't any
on the file system, and the script would pick it up. I know it's nitpicky
territory, and that's the reason I kept it silenced for a long time (instead
of fixing), but reporters are reporting this, and I figured if there is a proper,
non-intrusive way to do this, we can just do it. For other scripts it actually
made them simpler, more readable. If such option exist with qshell, it'd
be nice, if not, it's also fine, and if someone insist the comment can be
expanded to an explanation.

[monnerat]

OK, there are probably hidden behaviors there as the top find directory is the curl tree. I wouldn't go deeper on that.
Anyway if we hit on a name outside the curl's tree, we would get in trouble since it would be converted to UTF8 :-(

I started trying to imagine how we can do it, but qshell's xargs has no -d, -I options:
xargs: 001-3041 usage: xargs [-t] [-e[eofstring]] [-E eofstring] [-l[number]] [-L number] [-n number [-x]] [-s size] [utility [argument ...]]

[vszakats]

Zooming out a step, what is the reason to convert files to UTF-8?

curl sources have been purged from UTF-8 characters earlier this
year and are US-ASCII now, save a couple of exceptions:

The letter ö in lib/mqtt.*, and some UTF-8 in these misc text
files, AFAIU none necessary to build curl or its man pages:

.github/scripts/pyspelling.words
.mailmap
RELEASE-NOTES
docs/BINDINGS.md
docs/THANKS
docs/THANKS-filter
docs/GOVERNANCE.md
docs/TODO

If mqtt sources are the remaining blocker, and UTF-8 is making things
complex on OS/400, we may consider making that two file US-ASCII?
Would that help?

[monnerat]

Zooming out a step, what is the reason to convert files to UTF-8?

There are some Perl invokations during the build and the Perl interpreter is not an OS/400 program, but a PASE one.
PASE is an AIX emulation (a kind of subsystem runing outside OS/400) and thus ASCII-based, not EBCDIC. In addition it is CCSID-agnostic.
There once has been a native ILE OS/400 Perl interpreter a long time ago, but it has not been maintained (5.005_02: see https://www.cpan.org/ports/archive-2011-03-26.html#os400). In addition, it is not installed within the OS.
To make PASE Perl work, I have to be sure files are not EBCDIC. So I chose UTF-8.
In any case, an encoding conversion is needed because files created from OS/400 are encoded in some EBCDIC CCSID by default.

Would that help?

No I'm afraid, because we still need transcoding to an ASCII-based CCSID! But thanks anyway for the suggestion.