-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
cmd/deps: add --os
and --arch
#17122
Conversation
Signed-off-by: Michael Cho <[email protected]>
85eaa26
to
77a16c2
Compare
Is there any particular reason we can't add tests for this? |
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!
flag "--os=", | ||
description: "Show dependencies for the given operating system." | ||
flag "--arch=", | ||
description: "Show dependencies for the given CPU architecture." |
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.
Tangential: Do we list supported operating systems and architectures in the docs anywhere? If not, that might be a good thing to add at some point since this pattern is showing up in a few commands.
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.
I don't think it is documented. Closest may be on_*
docs - https://docs.brew.sh/Formula-Cookbook#handling-different-system-configurations
Supported architectures at least show up if unsupported one is passed in, e.g.
Error: New arch must be :arm or :intel
raise UsageError, "`brew deps --os=all` is not supported" if args.os == "all" | ||
raise UsageError, "`brew deps --arch=all` is not supported" if args.arch == "all" | ||
|
||
os, arch = T.must(args.os_arch_combinations.first) | ||
all = args.eval_all? | ||
|
||
Formulary.enable_factory_cache! |
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.
I double-checked to make sure that the caching strategy supports multiple platforms.
@dduugg My guess is that tests weren't added because they'd need to be integration tests or things would need to be refactored more. The change itself seems pretty straightforward if you look at the whitespace-insensitive diff. |
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.
Makes sense, thanks @cho-m!
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Personal use-case is viewing Linux dependency tree for a formula. These extra args provide a simpler way of accessing info rather than using Linux container/VM.