enhance(scripts/lint-packages.sh): make version check more robust. #26786
Conversation
e6cd26c to
aaf7e40
Compare
|
As @Biswa96 has made me aware of, the version check fails if there is no I'm too tired to test this fix fully right now. |
|
I tested this version, it seems to work great: It doesn't seem to take very long to run each check, but if this is also about optimizing the linter, then you're probably right that it would be more optimal to move the |
b378b88 to
9d2f699
Compare
I've moved it to above the function declaration with a comment explaining that we'll need it in the check. |
|
I tested this a bit, but I encountered a little bit of a strange problem, it seems like it might have encountered another edge case and compared with the wrong
If you want to reproduce this yourself and take a closer look at it, I might be able to provide some commands which would exactly reproduce this problem on your computer too, let me know if you would like that. |
9d2f699 to
e66e6b2
Compare
|
That latest linter failure doesn't make any sense. Oh... because the old version for |
e66e6b2 to
5044dca
Compare
|
I don't know if this fixed the git fetch issue yet, but |
5044dca to
d208782
Compare
| local new_revision="${version_new##*-}" old_revision="${version_old##*-}" | ||
|
|
||
| # If the version hasn't changed the revision must be incremented by 1 | ||
| # A decrease or no increase would have been caught above. | ||
| # But we want to additionally enforce sequential increase. | ||
| if [[ "${version_new%-*}" == "${version_old%-*}" && "$new_revision" != "$((old_revision + 1))" ]]; then | ||
| (( error++ )) # Not incremented sequentially | ||
| printf '%s\n' "FAILED " \ | ||
| "TERMUX_PKG_REVISION should be incremented sequentially" \ | ||
| "when a package is rebuilt with no new upstream release." \ | ||
| "" \ | ||
| "Got : ${version_old} -> ${version_new}" \ | ||
| "Expected: ${version_old} -> ${version}-$((old_revision + 1))" | ||
| continue | ||
| # If that check passed the TERMUX_PKG_VERSION must have changed, | ||
| # in which case TERMUX_PKG_REVISION should be reset to 0. | ||
| # This check isn't useful past the first index when $TERMUX_PKG_VERSION is an array | ||
| # since the main version of such a package may remain unchanged when another is changed. | ||
| elif [[ "${version_new%-*}" != "${version_old%-*}" && "$new_revision" != "0" && "$i" == 0 ]]; then | ||
| (( error++ )) # Not reset | ||
| printf '%s\n' \ | ||
| "FAILED - $version_old -> $version_new" \ | ||
| "" \ | ||
| "TERMUX_PKG_VERSION was bumped but TERMUX_PKG_REVISION wasn't reset." \ | ||
| "Please remove the 'TERMUX_PKG_REVISION=${new_revision}' line." \ | ||
| "" | ||
| continue | ||
| fi |
There was a problem hiding this comment.
I've added a check for $TERMUX_PKG_REVISION being properly reset or incremented.
fc9f4c6 to
2cc1c87
Compare
|
I finally figured out that git fetch issue. In other news.
Replacing that This puts the total speedup for a full linter sweep at about 575% at this point. |
robertkirkman
left a comment
There was a problem hiding this comment.
This is not having any more errors for me, and I think it is probably more reliable than the previous versions of this function.
robertkirkman
left a comment
There was a problem hiding this comment.
Wait, I performed another test just to check what would happen. This time I performed this test:
rm -rf termux-packages
docker container kill termux-package-builder
docker container rm termux-package-builder
git clone https://github.com/termux/termux-packages.git
cd termux-packages
curl https://patch-diff.githubusercontent.com/raw/termux/termux-packages/pull/26786.diff | git apply -v
git apply -v << 'EOF'
--- a/packages/grep/build.sh
+++ b/packages/grep/build.sh
@@ -18,3 +18,7 @@ gl_cv_func_setlocale_works=yes
"
# Avoid automagic dependency on libiconv
TERMUX_PKG_EXTRA_CONFIGURE_ARGS+=" am_cv_func_iconv=no"
+
+termux_step_pre_configure() {
+ echo "test"
+}
EOF
scripts/run-docker.sh scripts/lint-packages.sh packages/grep/build.shand this result happened:
Package: grep
Layout: PASS
Package name 'grep': PASS
End of line check: PASS
File permission check: PASS
Indentation check: PASS
Syntax check: PASS
Trailing whitespace check: PASS
TERMUX_PKG_HOMEPAGE: PASS
TERMUX_PKG_DESCRIPTION: PASS
TERMUX_PKG_LICENSE: PASS
TERMUX_PKG_MAINTAINER: PASS
TERMUX_PKG_API_LEVEL: PASS
TERMUX_PKG_VERSION: PASS - 3.12 (not modified in this branch)
TERMUX_PKG_REVISION: PASS
TERMUX_PKG_SRCURL: PASS
TERMUX_PKG_SHA256: PASS
TERMUX_PKG_ESSENTIAL: PASS
================================================================
Checked 1 packages.
Everything seems ok.
================================================================
Is this the expected behavior, or should grep fail the linter check because of having build.sh modified without version or revision also being modified?
That is not what should happen. termux-packages/scripts/lint-packages.sh Lines 146 to 156 in afb96ab The fix should be as simple as removing the |
…ll from checks and minor license sorting cleanup
377fa8d to
bb06339
Compare
robertkirkman
left a comment
There was a problem hiding this comment.
Ok, now I think it's really working
|
This should be good to go then, so let's get this merged and if we missed something else I'm sure we'll hear about it. |
|
Why is No Version Check removed? |
The idea seems to be that if the version check lint step is robust enough, then there should be no reason to turn it off, so turning it off should not be supported. However I could be wrong, @TomJo2000 do you think there is any case remaining where turning off the version check would still be necessary? |
Not that I'm aware of. |
|
Forks of the repo shouldnt care about checking version to be able to build packages on CI. This did not happen when the lint-packages.sh was just a simple format checker for build.sh. Now I dont think its a linter anymore. |
personally, I do agree and my personal opinion has always been that many of the things the |
At a certain point I asked Tomjo2000 about this, and I asked something like "can the version check be just turned off if it is detected that |
|
To what extent that should have to concern forks is something we can have a discussion about. The linter isn't there to beat contributors or forks over the head with unreasonable code style standards, it's there to catch issues early and ensure the established standards are being consistently applied. |
|
As far as I concern the linter should not demand changing package versions if the changes are not submitted to this repo or minor/chore in nature. This https://github.com/termux/termux-packages/blob/master/CONTRIBUTING.md#rebuilding-package-with-no-version-change should be rewritten to take account those scenarios. |
|
Running lint-packages.sh on device get blasted with these errors. I do not get how these can get approved. |
|
Not getting any of that locally. What are you running this on? |
|
Like I said, clone the repo and run |
|
If you did mention that above I did not catch that, sorry, I will be getting to the bottom of that as soon as I can. |
For example it wouldn't work on termux-pacman unless |
|
I can add a codepath for Say for example dpkg says |
and get some performance improvements by removing the
grepandsedcalls from the success path and replacing them with=~,==, andcaseas appropriate.I used a profile to identify the hottest parts of the script and it turns out that was
grepandsed.In my local testing this has yielded a ~450% performance increase for running a full linter sweep (
./scripts/lint-packages.sh, e.g. linting all packages).~4m10s before this PR.
~53s after.
The hottest part now is
dirname, but that's spread out over 8000+ calls, so that's not worth optimizing further.Here's the linter report of anyone's interested.
Nothing is hogging the script's time particularly badly so I'm happy to call it here for optimizations now.
I used this profiler I found on StackOverflow.
https://github.com/Kamilcuk/L_bash_profile