Skip to content
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

Require SystemCommand only where needed #16540

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

dduugg
Copy link
Member

@dduugg dduugg commented Jan 27, 2024

  • 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?

Continuation of #16538

This removes SystemCommand::Mixin from the top-level namespace, and instead includes it only in the needed namespaces.

@dduugg
Copy link
Member Author

dduugg commented Jan 27, 2024

Homebrew/homebrew-test-bot#1004 is an attempt to fix the failing check

@apainintheneck
Copy link
Contributor

It looks like this is also used in Casks which seems to be covered here and in two cask repo commands as well.

@bevanjkay @reitermarkus How hard would it be to replace these uses of system_command with other methods?

@dduugg
Copy link
Member Author

dduugg commented Jan 28, 2024

It looks like this is also used in Casks which seems to be covered here and in two cask repo commands as well.

@bevanjkay @reitermarkus How hard would it be to replace these uses of system_command with other methods?

It looks like we can require brew paths in this repo, e.g. https://github.com/Homebrew/homebrew-cask/blob/fb9cc3d4e7083471a99ae4808684d09458c9bcba/cmd/lib/generate-matrix.rb#L4

If so, we should be able to require "system_command" and include/extend SystemCommand::Mixin where appropriate. Does that seem possible?

@dduugg dduugg changed the title Require SystemInclude only where needed Require SystemCommand only where needed Jan 28, 2024
@Bo98
Copy link
Member

Bo98 commented Jan 28, 2024

@bevanjkay @reitermarkus How hard would it be to replace these uses of system_command with other methods?

They should probably be replaced with public API like Utils.safe_popen_read that's less likely to be volatile to changes. The SystemCommand::Mixin refactor here should largely be an internal thing (test-bot is OK).

@dduugg
Copy link
Member Author

dduugg commented Jan 31, 2024

It looks like this is also used in Casks which seems to be covered here and in two cask repo commands as well.

@bevanjkay @reitermarkus How hard would it be to replace these uses of system_command with other methods?

It looks like we can require brew paths in this repo, e.g. https://github.com/Homebrew/homebrew-cask/blob/fb9cc3d4e7083471a99ae4808684d09458c9bcba/cmd/lib/generate-matrix.rb#L4

If so, we should be able to require "system_command" and include/extend SystemCommand::Mixin where appropriate. Does that seem possible?

I went ahead and opened a PR to do just this: Homebrew/homebrew-cask#165732

@dduugg dduugg merged commit b33373a into Homebrew:master Jan 31, 2024
29 checks passed
@dduugg dduugg deleted the systemcommand-scope branch January 31, 2024 22:37
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants