Skip to content

Conversation

@SMillerDev
Copy link
Member

  • 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 lgtm (style, typechecking and tests) with your changes locally?

Copilot AI review requested due to automatic review settings January 9, 2026 21:40
@SMillerDev
Copy link
Member Author

Partially split from: #20334

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request enhances Cask CI matrix generation to correctly handle partial architecture dependencies, particularly when casks have OS-specific architecture requirements (e.g., requiring x86_64 on macOS but arm64 on Linux). The changes introduce per-OS architecture filtering to ensure CI runners are properly selected based on platform-specific constraints.

Key Changes

  • Modified the architectures method to accept an os parameter, enabling OS-specific architecture filtering based on simulated system environments
  • Updated filter_runners to separately filter macOS and Linux runners based on their respective architecture requirements
  • Added contains_os_specific_artifacts? method to detect casks with platform-specific artifacts that affect OS compatibility
  • Introduced comprehensive test coverage for various combinations of OS and architecture dependencies

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
Library/Homebrew/dev-cmd/generate-cask-ci-matrix.rb Enhanced architecture filtering to support OS-specific dependencies; modified filter_runners and architectures methods to handle per-OS architecture constraints
Library/Homebrew/cask/cask.rb Added contains_os_specific_artifacts? method to improve Linux support detection by checking for OS-specific artifact constraints
Library/Homebrew/cask/artifact.rb Added LINUX_ONLY_ARTIFACTS constant (empty placeholder for future Linux-specific artifacts)
Library/Homebrew/test/dev-cmd/generate-cask-ci-matrix_spec.rb Added comprehensive test cases covering various scenarios including mixed OS/arch dependencies, OS-specific dependencies, and system-wide constraints

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 175 to 203
sig { returns(T::Boolean) }
def contains_os_specific_artifacts?
return false unless @dsl.on_system_blocks_exist?

any_loaded = false
@contains_os_specific_artifacts ||= begin
OnSystem::VALID_OS_ARCH_TAGS.each do |bottle_tag|
Homebrew::SimulateSystem.with_tag(bottle_tag) do
refresh

any_loaded = true if artifacts.any? do |artifact|
(bottle_tag.linux? && ::Cask::Artifact::MACOS_ONLY_ARTIFACTS.include?(artifact.class)) ||
(bottle_tag.macos? && ::Cask::Artifact::LINUX_ONLY_ARTIFACTS.include?(artifact.class))
end
end
end

any_loaded
rescue CaskInvalidError
# Invalid cask
ensure
refresh
end

@dsl.os.present?
@contains_os_specific_artifacts
end
Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new contains_os_specific_artifacts? method lacks test coverage. Given that Library/Homebrew/test/cask/cask_spec.rb exists with comprehensive tests for other Cask methods, this new method should have corresponding test cases to verify its behavior across different scenarios (e.g., casks with OS-specific artifacts, casks without, error cases).

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it'd be good to have even just one very simple test for this. Could ask Copilot to try it for you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few small comments.

Comment on lines 175 to 203
sig { returns(T::Boolean) }
def contains_os_specific_artifacts?
return false unless @dsl.on_system_blocks_exist?

any_loaded = false
@contains_os_specific_artifacts ||= begin
OnSystem::VALID_OS_ARCH_TAGS.each do |bottle_tag|
Homebrew::SimulateSystem.with_tag(bottle_tag) do
refresh

any_loaded = true if artifacts.any? do |artifact|
(bottle_tag.linux? && ::Cask::Artifact::MACOS_ONLY_ARTIFACTS.include?(artifact.class)) ||
(bottle_tag.macos? && ::Cask::Artifact::LINUX_ONLY_ARTIFACTS.include?(artifact.class))
end
end
end

any_loaded
rescue CaskInvalidError
# Invalid cask
ensure
refresh
end

@dsl.os.present?
@contains_os_specific_artifacts
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it'd be good to have even just one very simple test for this. Could ask Copilot to try it for you.

SMillerDev and others added 3 commits January 20, 2026 18:58
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI commented Jan 20, 2026

@SMillerDev I've opened a new pull request, #21432, to work on those changes. Once the pull request is ready, I'll request review from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants