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

Make utilities posix compatible #15139

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nanobowers
Copy link

Replace cp -av with cp -R -P -p because neither -a nor -v are POSIX compliant (Makefile)

Replace local keyword in shell functions with subshell style ( ) functions. (bin/crystal)

Fixed other Shellcheck warnings. (bin/crystal)

Fixes #14846

Replace `cp -av` with `cp -R -P -p` because neither `-a` nor `-v` are POSIX compliant

Replace `local` keyword in shell functions with subshell style `( )` functions.  Fixed other Shellcheck warnings.
@HertzDevil
Copy link
Contributor

HertzDevil commented Oct 30, 2024

I'm not completely sure dropping -v is a good idea, so maybe some form of find | xargs cp would work better? Would that be feasible?

@straight-shoota
Copy link
Member

straight-shoota commented Oct 30, 2024

Just the other day I've been wondering about dropping -v anyway. It's quite noisy with 1.5k files in src/ and 700 files in docs/. That makes it hard to scroll through the log and discover more relevant output before or after the cp commands.
I suppose it might be useful sometimes to have a full log of all copied files, but that seems quite rare to me.
So I'm fine with dropping it.
We usually have a clear reference point (a git commit) and should not expect cp to make any big mistakes in copying files that would require an investigation of the copy log.

@straight-shoota straight-shoota added this to the 1.15.0 milestone Oct 30, 2024
@straight-shoota
Copy link
Member

straight-shoota commented Oct 31, 2024

@nanobowers @HertzDevil I resolved a merge conflict with #15138 pretty much to the letter (merging -R and -r which should have the same meaning).
I presume we should also unify --preserve=all and -p (which equals --preserve=mode,ownership,timestamp)?

@nanobowers
Copy link
Author

@nanobowers @HertzDevil I resolved a merge conflict with #15138 pretty much to the letter (merging -R and -r which should have the same meaning).

FWIW, I wasn't sure what to do with -r vs -R. From the documentation I found, it looks like -r is deprecated and hopefully the differences are a don't care for Crystal.

https://pubs.opengroup.org/onlinepubs/9799919799/

Earlier versions of this standard included support for the -r option to copy file hierarchies. The -r option is historical practice on BSD and BSD-derived systems. This option is no longer specified by POSIX.1-2024 but may be present in some implementations. The -R option was added as a close synonym to the -r option, selected for consistency with all other options in this volume of POSIX.1-2024 that do recursive directory descent.

The difference between -R and the removed -r option is in the treatment by cp of file types other than regular and directory. It was implementation-defined how the - option treated special files to allow both historical implementations and those that chose to support -r with the same abilities as -R defined by this volume of POSIX.1-2024. The original -r flag, for historic reasons, did not handle special files any differently from regular files, but always read the file and copied its contents. This had obvious problems in the presence of special file types; for example, character devices, FIFOs, and sockets.

I presume we should also unify --preserve=all and -p (which equals --preserve=mode,ownership,timestamp)?

I missed that one, thanks! Fix is incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strict POSIX conformance in utility files
4 participants