cask/audit: add audit_min_os #16013

razvanazamfirei · 2023-09-18T16:45:44Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

This PR adds an audit to ensure that the minimum macOS version specified in the cask matches the minimum macOS version specified by the developer.

It does so by:

Hijacking audit_livecheck_min_os to obtain the sparkle version information.
Adding audit_plist_min_os to extract the version information from Info.plist.
Comparing the cask min_os version with the extracted value. If both a sparkle min_os and plist min_os are present, we take the larger value of the two.

Note: This will break a lot of casks.

Bo98 · 2023-09-18T17:21:37Z

Library/Homebrew/cask/audit.rb

+    def audit_min_os
+      odebug "Auditing minimum OS version"
+
+      sparkle_min_os = audit_livecheck_min_os
+      plist_min_os = audit_plist_min_os
+      min_os_string = [sparkle_min_os, plist_min_os].max


Worth noting that this will run as three audits as each audit is automatically run via looking for audit_-prefixed methods. It seems the intention here is to have one audit that calls these two methods, in which case you will have to rename these to something different and just have audit_min_os with the audit_ prefix

apainintheneck

Just out of curiosity, do you have guesstimates for the number of casks that will fail this audit and how long it will take to run on average? I believe the cask repo runs online audits by default, right?

apainintheneck · 2023-09-19T03:29:43Z

Library/Homebrew/cask/audit.rb

+          path = tmpdir/artifact_path.relative_path_from(cask.staged_path)
+
+          next unless path.exist?
+
+          plist_path = "#{path}/Contents/Info.plist"
+          next unless File.exist?(plist_path)


Nit: We probably don't need the path check if we're checking for the existence of the file later on.

There will probably be a large number of failures, since up until now we've only had an audit for Sparkle. Everything else has either been checking the .plist manually when the cask is added or referring to documentation.

razvanazamfirei · 2023-09-23T14:07:43Z

I'm incorporating the comments made by people, but also slowly going through casks and updating min_os versions. If we end up implementing something akin to this audit, I think the effort required to update all the casks won't be trivial.

@apainintheneck

Number of casks affected:
I ran the audit on homebrew/cask-versions and 33.6% (81/241) of casks failed. I would imagine this number to be similar, if not higher for the main tap.

Time to run:
As part of the full audit, it doesn't seem to make much of a difference. Ran individually, it seems to add 5 seconds, but I didn't have a good audit to benchmark this against. In terms of running it for all the casks, ~11 hours excluding download times.

❯ hyperfine 'brew audit --cask --online --strict quickgpt' --prepare 'git checkou
t cask-audit-min-os' 'brew audit --cask --online --strict quickgpt'
Benchmark 1: brew audit --cask --online --strict quickgpt [old]
  Time (mean ± σ):     20.708 s ±  1.540 s    [User: 2.400 s, System: 1.209 s]
  Range (min … max):   18.472 s … 23.800 s    10 runs

Benchmark 2: brew audit --cask --online --strict quickgpt [new]
  Time (mean ± σ):     20.647 s ±  4.108 s    [User: 2.360 s, System: 1.134 s]
  Range (min … max):   12.560 s … 24.493 s    10 runs

❯ hyperfine 'brew audit --only \'name\' quickgpt --strict --online' 'brew audit -
-only \'min_os, name\' quickgpt --strict --online'
Benchmark 1: brew audit --only 'name' quickgpt --strict --online
  Time (mean ± σ):      3.245 s ±  0.223 s    [User: 1.883 s, System: 0.542 s]
  Range (min … max):    3.023 s …  3.709 s    10 runs

Benchmark 2: brew audit --only 'min_os, name' quickgpt --strict --online
  Time (mean ± σ):      8.001 s ±  0.829 s    [User: 2.193 s, System: 0.848 s]
  Range (min … max):    7.052 s …  9.655 s    10 runs

apainintheneck · 2023-09-23T18:01:35Z

Thanks for the benchmarks. It doesn't look like it'll be a performance problem.

Like you already noted the number of casks this affects is large enough that it will take a lot of work to update all of them. Maybe it'd be better to just apply this to new cask updates so that we can ship it faster and then maybe enable it for all casks later on as a follow-up.

p-linnane · 2023-09-23T18:03:09Z

Like you already noted the number of casks this affects is large enough that it will take a lot of work to update all of them. Maybe it'd be better to just apply this to new cask updates so that we can ship it faster and then maybe enable it for all casks later on as a follow-up.

This makes the most sense. Once it's merged we can create a tracking issue like we've done for our other projects of this scale.

MikeMcQuaid · 2023-09-25T11:56:20Z

Like you already noted the number of casks this affects is large enough that it will take a lot of work to update all of them. Maybe it'd be better to just apply this to new cask updates so that we can ship it faster and then maybe enable it for all casks later on as a follow-up.

Agreed. I think making it a --strict and/or --new-cask only audit makes sense until it's fully implemented.

MikeMcQuaid

Looking good so far! Have some mild performance concerns but they aren't necessarily specific to this check so could be non-blocking if you'd rather tackle them later.

Library/Homebrew/cask/audit.rb

MikeMcQuaid · 2023-10-04T07:51:09Z

Library/Homebrew/cask/audit.rb

+        artifacts.each do |artifact|
+          artifact_path = artifact.is_a?(Artifact::Pkg) ? artifact.path : artifact.source
+          path = tmpdir/artifact_path.relative_path_from(cask.staged_path)
+          plist_path = "#{path}/Contents/Info.plist"


Can this somehow be the only file that's extracted?

There's some overlap between this method and audit_signing and perhaps it could make sense to have a common method for both? Otherwise, I'm happy to take a look at fixing this later on.

@razvanazamfirei any more thoughts on this? I don't know if there's decent enough support in these classes for partial extraction, ok if not, but nice to avoid extracting the entire archive if we can avoid it.

Library/Homebrew/cask/audit.rb

github-actions · 2023-10-26T00:06:09Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

MikeMcQuaid · 2023-11-01T13:58:43Z

Library/Homebrew/cask/audit.rb

+        artifacts.each do |artifact|
+          artifact_path = artifact.is_a?(Artifact::Pkg) ? artifact.path : artifact.source
+          path = tmpdir/artifact_path.relative_path_from(cask.staged_path)
+          plist_path = "#{path}/Contents/Info.plist"


@razvanazamfirei any more thoughts on this? I don't know if there's decent enough support in these classes for partial extraction, ok if not, but nice to avoid extracting the entire archive if we can avoid it.

MikeMcQuaid · 2023-11-01T14:01:19Z

Library/Homebrew/cask/audit.rb

+      sparkle_min_os = livecheck_min_os
+      plist_min_os = cask_plist_min_os
+      odebug "Minimum OS version: Plist #{plist_min_os} | Sparkle #{sparkle_min_os.inspect}"
+      min_os_string = [sparkle_min_os, plist_min_os].compact.max


I don't think .max will work like how you want/expect on two strings. You probably need to do Version.new on them both and then compare them instead. String version comparisons can look like they work sometimes but are not worth relying on.

@MikeMcQuaid, I did not find great support for the partial extraction. I tried to deduplicate the extraction operation that was duplicated in the signing audit and the minimum OS audit.

Both livecheck_min_os and plist_min_os are a MacOSVersion.

MikeMcQuaid

Thanks again @razvanazamfirei, sorry for all the rounds of review!

Bo98 reviewed Sep 18, 2023

View reviewed changes

apainintheneck reviewed Sep 19, 2023

View reviewed changes

razvanazamfirei mentioned this pull request Sep 23, 2023

various: add min os Homebrew/homebrew-cask-versions#17813

Merged

razvanazamfirei force-pushed the cask-audit-min-os branch 3 times, most recently from 93feb2f to 4b7bf5d Compare October 3, 2023 15:46

razvanazamfirei marked this pull request as ready for review October 3, 2023 15:46

MikeMcQuaid reviewed Oct 4, 2023

View reviewed changes

github-actions bot added the stale No recent activity label Oct 26, 2023

razvanazamfirei force-pushed the cask-audit-min-os branch from 4b7bf5d to 800a9f1 Compare October 26, 2023 19:45

razvanazamfirei removed the stale No recent activity label Oct 26, 2023

razvanazamfirei requested a review from MikeMcQuaid November 1, 2023 13:44

MikeMcQuaid reviewed Nov 1, 2023

View reviewed changes

razvanazamfirei added 3 commits November 5, 2023 17:18

cask/audit: add audit_min_os

c90e6e7

audit: code review changes

e605237

audit: add extract artifcats method

df2aee4

razvanazamfirei force-pushed the cask-audit-min-os branch from 800a9f1 to df2aee4 Compare November 5, 2023 22:18

MikeMcQuaid approved these changes Nov 6, 2023

View reviewed changes

MikeMcQuaid merged commit fce60ee into Homebrew:master Nov 6, 2023
27 checks passed

samford mentioned this pull request Nov 8, 2023

Use Sparkle sorting/filtering in #livecheck_min_os #16196

Merged

7 tasks

github-actions bot added the outdated PR was locked due to age label Dec 7, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2023

razvanazamfirei deleted the cask-audit-min-os branch January 28, 2024 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cask/audit: add audit_min_os #16013

cask/audit: add audit_min_os #16013

razvanazamfirei commented Sep 18, 2023 •

edited

Loading

Bo98 Sep 18, 2023

apainintheneck left a comment

apainintheneck Sep 19, 2023

p-linnane Sep 21, 2023 •

edited

Loading

razvanazamfirei commented Sep 23, 2023

apainintheneck commented Sep 23, 2023

p-linnane commented Sep 23, 2023

MikeMcQuaid commented Sep 25, 2023

MikeMcQuaid left a comment

MikeMcQuaid Oct 4, 2023

razvanazamfirei Oct 26, 2023

MikeMcQuaid Nov 1, 2023

github-actions bot commented Oct 26, 2023

MikeMcQuaid Nov 1, 2023

MikeMcQuaid Nov 1, 2023

razvanazamfirei Nov 5, 2023

MikeMcQuaid left a comment

cask/audit: add audit_min_os #16013

cask/audit: add audit_min_os #16013

Conversation

razvanazamfirei commented Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

apainintheneck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-linnane Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

razvanazamfirei commented Sep 23, 2023

apainintheneck commented Sep 23, 2023

p-linnane commented Sep 23, 2023

MikeMcQuaid commented Sep 25, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

razvanazamfirei commented Sep 18, 2023 •

edited

Loading

p-linnane Sep 21, 2023 •

edited

Loading