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

pull: Fix pull --bottle 1234 #3337

Merged
merged 1 commit into from Oct 22, 2017

Conversation

Projects
None yet
4 participants
@sjackman
Contributor

sjackman commented Oct 19, 2017

  • 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 tests with your changes locally?

Fix the error:

Error: undefined method `casecmp' for nil:NilClass
@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 19, 2017

Contributor

This PR fixes a bug that I introduced in PR #3279. Sorry for that.

Contributor

sjackman commented Oct 19, 2017

This PR fixes a bug that I introduced in PR #3279. Sorry for that.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 19, 2017

Member

@sjackman This is failing on the Linux CI. Note the Linux CI should not end up cloning Linuxbrew in any situation.

Member

MikeMcQuaid commented Oct 19, 2017

@sjackman This is failing on the Linux CI. Note the Linux CI should not end up cloning Linuxbrew in any situation.

@scpeters

This comment has been minimized.

Show comment
Hide comment
@scpeters

scpeters Oct 19, 2017

Contributor

Note the Linux CI should not end up cloning Linuxbrew in any situation.

I see references to linuxbrew repositories in the following places:

BREW_OFFICIAL_REMOTE="https://github.com/Linuxbrew/brew"
CORE_OFFICIAL_REMOTE="https://github.com/Linuxbrew/homebrew-core"

"https://github.com/Linuxbrew/homebrew-core".freeze

Contributor

scpeters commented Oct 19, 2017

Note the Linux CI should not end up cloning Linuxbrew in any situation.

I see references to linuxbrew repositories in the following places:

BREW_OFFICIAL_REMOTE="https://github.com/Linuxbrew/brew"
CORE_OFFICIAL_REMOTE="https://github.com/Linuxbrew/homebrew-core"

"https://github.com/Linuxbrew/homebrew-core".freeze

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 19, 2017

Member

@scpeters Yep, those are fine but we've previously taken lengths to workaround that in CI itself.

Member

MikeMcQuaid commented Oct 19, 2017

@scpeters Yep, those are fine but we've previously taken lengths to workaround that in CI itself.

@JCount

This comment has been minimized.

Show comment
Hide comment
@JCount

JCount Oct 19, 2017

Member

@sjackman
The CI issue seems caused by the use of CoreTap's default_remote method, due to its platform-specific behavior. This is likely cause for the test suite to cloning https://github.com/Linuxbrew/homebrew-core when it runs brew pull 1 as part of the pull_spec tests. I suspect that

arg = "#{CoreTap.instance.default_remote}/pull/#{arg}" if arg.to_i.positive?

is evaluating to

arg = "https://github.com/Linuxbrew/homebrew-core/pull/1"

on the Linux CI.

Member

JCount commented Oct 19, 2017

@sjackman
The CI issue seems caused by the use of CoreTap's default_remote method, due to its platform-specific behavior. This is likely cause for the test suite to cloning https://github.com/Linuxbrew/homebrew-core when it runs brew pull 1 as part of the pull_spec tests. I suspect that

arg = "#{CoreTap.instance.default_remote}/pull/#{arg}" if arg.to_i.positive?

is evaluating to

arg = "https://github.com/Linuxbrew/homebrew-core/pull/1"

on the Linux CI.

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 21, 2017

Contributor

I've fixed the test failure. I've removed the tap name synonym Homebrew/homebrew for Homebrew/core. I can put it back in if you prefer.

Contributor

sjackman commented Oct 21, 2017

I've fixed the test failure. I've removed the tap name synonym Homebrew/homebrew for Homebrew/core. I can put it back in if you prefer.

Show outdated Hide outdated Library/Homebrew/tap.rb Outdated
@@ -22,7 +22,7 @@
.and output(/Current branch is new\-branch/).to_stderr
.and be_a_failure
expect { brew "pull", "--bump", "8" }
expect { brew "pull", "--bump", "https://github.com/Homebrew/homebrew-core/pull/8" }

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 22, 2017

Member

To be explicit: this is just to fix the test failure on Linux, right? Using 8 will still pull Homebrew/homebrew-core on macOS?

@MikeMcQuaid

MikeMcQuaid Oct 22, 2017

Member

To be explicit: this is just to fix the test failure on Linux, right? Using 8 will still pull Homebrew/homebrew-core on macOS?

This comment has been minimized.

@sjackman

sjackman Oct 22, 2017

Contributor

Correct.

@sjackman

sjackman Oct 22, 2017

Contributor

Correct.

Show outdated Hide outdated Library/Homebrew/tap.rb Outdated
pull: Fix pull --bottle 1234
Fix the error:
Error: undefined method `casecmp' for nil:NilClass

@MikeMcQuaid MikeMcQuaid merged commit 7e4f423 into Homebrew:master Oct 22, 2017

3 checks passed

codecov/patch 100% of diff hit (target 68.22%)
Details
codecov/project Absolute coverage decreased by -1.16% but relative coverage increased by +31.77% compared to ce0e96c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 22, 2017

Member

Thanks again @sjackman and sorry for all the back and forth.

Member

MikeMcQuaid commented Oct 22, 2017

Thanks again @sjackman and sorry for all the back and forth.

@sjackman

This comment has been minimized.

Show comment
Hide comment
@sjackman

sjackman Oct 22, 2017

Contributor

No worries. Thanks for merging, Mike.

Contributor

sjackman commented Oct 22, 2017

No worries. Thanks for merging, Mike.

@sjackman sjackman deleted the sjackman:pull branch Oct 22, 2017

@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.