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

` is deprecated #273

Closed
wants to merge 2 commits into from
Closed

Conversation

arch-user-france1
Copy link

@arch-user-france1 arch-user-france1 commented May 31, 2022

` is deprecated (ref: https://wiki.bash-hackers.org/scripting/obsolete and the script called shellcheck). I replaced them with $().

` is deprecated (ref: https://wiki.bash-hackers.org/scripting/obsolete). I replaced them with $().
@CLAassistant
Copy link

CLAassistant commented May 31, 2022

CLA assistant check
All committers have signed the CLA.

I replaced them with $(nproc) but accidentally put in a space (which does not break the code but changes the layout/stile) so I removed that one right now.
@aritger aritger self-assigned this May 31, 2022
@YusufKhan-gamedev
Copy link

YusufKhan-gamedev commented Jun 1, 2022

My shell program removed this a long time ago. Acked-by: Yusuf Khan <yusisamerican AT gmail DOT com>

@arch-user-france1
Copy link
Author

My shell program removed this a long time ago. Acked-by: Yusuf Khan <yusisamerican AT gmail DOT com>

Note that it just is not recommended to use but it will stay there. No plans for removing it.

@MilesBHuff
Copy link

MilesBHuff commented Jun 14, 2022

Backticks may be deprecated in bash, but they are certainly not so universally. $() is not POSIX-compliant, which means it is not portable. Backticks should be kept unless we are talking about a script that requires bashisms; this PR should be rejected.
@aritger

@arch-user-france1
Copy link
Author

Okay, well then, I will close it.

@aritger
Copy link
Collaborator

aritger commented Jun 28, 2022

@MilesBHuff: we had actually already pulled this change into our code base, and it is now included in the 515.57 release. Do you have an example shell that doesn't honor the $() syntax?

@BratishkaErik
Copy link

$() is not POSIX-compliant, which means it is not portable.

I thought this is POSIX-compliant https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03

@PAR2020 PAR2020 added this to the 515.57 milestone Jun 29, 2022
@aritger
Copy link
Collaborator

aritger commented Jun 29, 2022

@arch-user-france1: note that we already applied this change to our tree, and it is present in 515.57. I don't think there is anything else to be done in this pull request unless someone can point to a shell that doesn't support the $(nproc) syntax, and then we would need to revert the change.

I'm going to go ahead and re-close this. Let me know if I've misunderstood.

@aritger aritger closed this Jun 29, 2022
@PAR2020 PAR2020 added the Implemented Fixed, in test prior to release integration label Jun 29, 2022
@MilesBHuff
Copy link

MilesBHuff commented Jul 2, 2022

@aritger

Do you have an example shell that doesn't honor the $() syntax?

The original Bourne shell.
Afaict, everything modern supports it -- even the minimalist Debian Almquist Shell.

@MilesBHuff
Copy link

MilesBHuff commented Jul 2, 2022

@BratishkaErik

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_03

Interesting! I stand corrected, then: I guess it is POSIX-compliant, even though some older shells don't support it. I apologize for the misinformation.

@birdie-github
Copy link

It's been deprecated for years if not a decade, it's not gonna get removed now or ever. I wouldn't say this merge request is useless but there's a scent of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Fixed, in test prior to release integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants