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

Define a custom assertion failure error for cross-version compat #2841

Merged
merged 3 commits into from
Jul 7, 2017

Conversation

mistydemeo
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 successfully run brew tests with your changes locally?

Fixes #2840.

Note that this partially reverts c45cca8 - it was unnecessary on Ruby 2.0, but turns out to be necessary for compatibility with the system Ruby 2.3 on OS X 10.13.

@mistydemeo mistydemeo requested a review from MikeMcQuaid June 30, 2017 00:44
@MikeMcQuaid
Copy link
Member

Note that this partially reverts c45cca8 - it was unnecessary on Ruby 2.0, but turns out to be necessary for compatibility with the system Ruby 2.3 on OS X 10.13.

I was happy to leave that code as-is until it broke brew tests under Bundler 1.15.1. Can you ensure that's not the case now?

My main issue with the previous code was the lack of documentation: it was unclear in what situation any of those ifs would occur. I'd like to see that addressed if we merge this.

I think the ideal situation, though, would be using the same module names regardless of the version. This should be possible for brew tests at least by ensuring it's using the same version of e.g. minitest everywhere. It may not be possible on Ruby 2.3 for formulae so this may be acceptable as a short-term workaround but before High Sierra is released I think I'd like to see us upgrade to a vendored Ruby 2.3.X (matching the High Sierra version). Differing Ruby versions was an absolute nightmare between 1.8 and 2.0 and now we've got a solution I'd love to see us lean into it.

@mistydemeo
Copy link
Member Author

it was unclear in what situation any of those ifs would occur. I'd like to see that addressed if we merge this.

Will do!

before High Sierra is released I think I'd like to see us upgrade to a vendored Ruby 2.3.X (matching the High Sierra version).

The static build for Ruby 2.3 is currently broken: https://bugs.ruby-lang.org/issues/13413 I'm going to take a stab at it, but it's a bit arcane.

@ilovezfs
Copy link
Contributor

ilovezfs commented Jul 6, 2017

The static build for Ruby 2.3 is currently broken: https://bugs.ruby-lang.org/issues/13413 I'm going to take a stab at it, but it's a bit arcane.

I wonder if upstream would be willing to add static builds to their CI so that this doesn't happen again, assuming it does get fixed at some point.

@MikeMcQuaid MikeMcQuaid merged commit ddb1fd7 into Homebrew:master Jul 7, 2017
@MikeMcQuaid
Copy link
Member

Thanks @mistydemeo!

@mistydemeo mistydemeo deleted the formula_assertions_constant branch July 10, 2017 19:46
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants