Build scripts: Use file name for grep directly#2397
Build scripts: Use file name for grep directly#2397ann0see merged 1 commit intojamulussoftware:masterfrom
Conversation
hoffie
left a comment
hoffie
left a comment
There was a problem hiding this comment.
Heh, micro-optimizations. :)
Meta: Both the PR title and the commit title could use some context in order for git log --oneline to be any useful. A shortened file name might do? build scripts: Use file name for grep directly
ann0see
force-pushed
the
feature/noUseCat
branch
2 times, most recently
from
f895363 to
d89e327
Compare
Code Style: Do not use cat | grep due to performance reasons: https://bencane.com/2013/08/19/grepping-a-file-without-using-cat-and-grep-other-tricks/ Found by Codacy
ann0see
force-pushed
the
feature/noUseCat
branch
from
d89e327 to
3a63033
Compare
|
Changed some more in mac/deploy_mac.sh and changed the commit name too |
ann0see
changed the title
| s) | ||
| cert_name=$OPTARG | ||
| if [[ -z "$cert_name" ]]; then | ||
| echo "Please add the name of the certificate to use: -s \"<name>\"" | ||
| fi | ||
| # shift 2 | ||
| ;; | ||
| h) | ||
| echo "Usage: -s <cert name> for signing mac build" | ||
| exit 0 | ||
| h) | ||
| echo "Usage: -s <cert name> for signing mac build" | ||
| exit 0 | ||
| ;; | ||
| *) | ||
| exit 1 | ||
| ;; | ||
|
|
There was a problem hiding this comment.
When working on style in this script anyway, then I'll add this here as well: ;)
I'd indent like described in this style guide (but with 4 spaces instead of 2, similar to what we do for if/while etc.)
There was a problem hiding this comment.
There was a problem hiding this comment.
This means there will be some border cases this Python program won't be able to process. But in tests with large Linux system Bash scripts, its error-free score was ~99%.
Still sounds scary
There was a problem hiding this comment.
Concerning indentation: I think this should be in #2402
There was a problem hiding this comment.
It's now in the mentioned PR anyway (as own commit)
|
Should be merged after upcoming release |
|
|
||
| # Get Jamulus version | ||
| local app_version="$(cat "${project_path}" | sed -nE 's/^VERSION *= *(.*)$/\1/p')" | ||
| local app_version="$(grep -oP 'VERSION = \K\w[^\s\\]*' Jamulus.pro)" |
There was a problem hiding this comment.
This change from sed -nE to grep -oP breaks on Mac. See https://github.com/jamulussoftware/jamulus/runs/5247842643?check_suite_focus=true#step:6:1652
I've filed #2421 to fix this.
|
CHANGELOG: Build: Improve grep usage in scripts #2421 |
|
Really sorry that you needed to fix my PRs @hoffie but thanks for finding the bugs. |
3 participants
Short description of changes
Code Style: Do not use cat | grep due to performance reasons: https://bencane.com/2013/08/19/grepping-a-file-without-using-cat-and-grep-other-tricks/
Found by Codacy
Context: Fixes an issue?
No. Just code style
Does this change need documentation? What needs to be documented and how?
No
Status of this Pull Request
Waiting on CI
What is missing until this pull request can be merged?
Reviewing.
Checklist