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
[hack/verify-file-sizes.sh] Support Mac OS X #124156
[hack/verify-file-sizes.sh] Support Mac OS X #124156
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Apr 2 14:19:09 UTC 2024. |
/sig testing |
/cc @pohly |
hack/verify-file-sizes.sh
Outdated
local file="$1" | ||
case "$(uname -s)" in | ||
Darwin) | ||
stat -f %z "$file" | tr -d '\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this line works well on Linux, why not use it directly in Line 61?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlory Thank you for your review.
Regarding the options for the stat command, I tried it on Linux, and here's what I found:
$ touch samplefile
$ stat -f %z samplefile | tr -d '\n'
stat: cannot read file system information for '%z': No such file or directory
File: "samplefile" ID: 0 Namelen: 255 Type: fuseblkBlock size: 1048576 Fundamental block size: 4096Blocks: Total: 59840624 Free: 18538697 Available: 18538697Inodes: Total: 744080125 Free: 741547880
$ stat --printf %z ./samplefile | tr -d '\n'
2024-04-03 03:25:17.374582976 +0000
$ stat --printf %s ./samplefile
0
As shown above, an error occurred with stat -f %z
, so I am switching options and such depending on whether the environment is Linux or Mac OS X.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sort of thing tends to cause problems because even on darwin you may have gnu coreutils installed, check e.g. when we require gnu-sed.
wc -c < "$file"
would work portably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenTheElder Thank you for your comment.
I have changed the implementation to use wc -c < "$file"
. Please review it when you have time.
Just to be safe, I've attached the test results below after verifying the operation on both Mac OS X and Linux:
Mac OS X
echo "abc" > samplefile
size=$(wc -c < samplefile)
echo "size: $size"
for i in 3 4
do
maxsize=$i
if [ "${size}" -gt "$maxsize" ]; then
echo "size is greater than maxsize"
else
echo "size is less than or equal to maxsize"
fi
done
size: 4
size is greater than maxsize
size is less than or equal to maxsize
Linux
echo "abc" > samplefile
size=$(wc -c < samplefile)
echo "size: $size"
for i in 3 4
do
maxsize=$i
if [ "${size}" -gt "$maxsize" ]; then
echo "size is greater than maxsize"
else
echo "size is less than or equal to maxsize"
fi
done
size: 4
size is greater than maxsize
size is less than or equal to maxsize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also confirming:
this test took
this pr: 3s: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/124156/pull-kubernetes-verify/1781407536046411776
random PR without this change: 6s: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/119589/pull-kubernetes-verify/1781378276028059648
so no performance issues
/triage accepted |
/priority backlog |
…Mac OS X with a single implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
thanks!
LGTM label has been added. Git tree hash: d1ec4431fac282a9a5dd5fc3269ca41f077e39a4
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bells17, BenTheElder The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
/sig testing
What this PR does / why we need it:
Enable the
hack/verify-file-sizes.sh
script to run successfully on Mac OS X.Which issue(s) this PR fixes:
Fixes #124155
Ref: #121549
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: