Skip to content

doctor: disable python symlink message#10184

Merged
Rylan12 merged 1 commit intoHomebrew:masterfrom
Rylan12:disable-python-doctor-message
Dec 31, 2020
Merged

doctor: disable python symlink message#10184
Rylan12 merged 1 commit intoHomebrew:masterfrom
Rylan12:disable-python-doctor-message

Conversation

@Rylan12
Copy link
Copy Markdown
Member

@Rylan12 Rylan12 commented Dec 30, 2020

  • 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?
  • Have you successfully run brew man locally and committed any changes?

brew doctor will soon start to fail on ubuntu-latest actions runs once they migrate to Ubuntu 20.04. This has already happened to me in personal taps.

The issue is that Ubuntu 20.04 no longer comes with Python 2. Instead, the python executable points to Python 3.8.5.

I think there are three options:

  1. Use ubuntu-18.04 instead of ubuntu-latest on all our runners. This isn't a good long-term solution
  2. Conditionally allow this doctor failure on CI machines to allow them to continue running
  3. Disable this check (everywhere or only on Linux) (suggested to me on Slack by @sjackman)

I don't know much about Homebrew on Linux, but I think that option 3 is the best option (mainly because @sjackman suggested it 😄). Ideally, now that Python 2 is no longer supported, we should expect to not deal with it in the future. Unfortunately, I'm not sure that's feasible on macOS (especially not yet).

@Rylan12 Rylan12 added the linux label Dec 30, 2020
@Rylan12 Rylan12 requested a review from sjackman December 30, 2020 16:28
@BrewTestBot
Copy link
Copy Markdown
Contributor

Review period will end on 2020-12-31 at 16:28:40 UTC.

1 similar comment
@BrewTestBot
Copy link
Copy Markdown
Contributor

Review period will end on 2020-12-31 at 16:28:40 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 30, 2020
@iMichka
Copy link
Copy Markdown
Member

iMichka commented Dec 30, 2020

I think this needs to be implemented in extend/os/linux/diagnostic.rb instead. We separated the linux from the Mac implementation with 2 different files.

@Rylan12
Copy link
Copy Markdown
Member Author

Rylan12 commented Dec 30, 2020

Makes sense. I'll just move the check to extend/os/mac/disagnostic.rb. That way no need forreturn if OS.linux?

Before I do, though, I'm going to change the ubuntu runner to ubuntu-20.04 to make it easier to verify that the change is effective in this PR.

@Rylan12
Copy link
Copy Markdown
Member Author

Rylan12 commented Dec 30, 2020

CI (on ubuntu-20.04) is now failing with the following message:

In /home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/utils/ruby.sh line 20:
    for ruby_exec in $(which -a ruby 2>/dev/null) $(PATH=$HOMEBREW_PATH which -a ruby 2>/dev/null)
                       ^---^ SC2230: which is non-standard. Use builtin 'command -v' instead.
                                                                        ^---^ SC2230: which is non-standard. Use builtin 'command -v' instead.

For more information:
  https://www.shellcheck.net/wiki/SC2230 -- which is non-standard. Use builti...
Error: which is non-standard. Use builtin 'command -v' instead.

According to https://www.shellcheck.net/wiki/SC2230, this is opt-in only and I don't believe we've opted in... Odd that this is only the case on Ubuntu 20.04.

@Rylan12
Copy link
Copy Markdown
Member Author

Rylan12 commented Dec 30, 2020

Thinking out loud here, is this the right solution? Since the brew docker container uses 20.04 and doesn't include a python executable, should taps just use that brew docker container?

Interestingly, brew doctor didn't fail when running on ubuntu-20.04 before I removed the check (see, for example, this action run). It doesn't look to me like we're using the docker container for the tap-syntax job. That makes me think that the issue is just an issue with my configuration...

I'm not a Linux guy and I've almost never used docker so I'm a tad out of my comfort zone here. Let me know if I'm misunderstanding the situation.

@Rylan12 Rylan12 changed the title doctor: disable python symlink message on Linux doctor: disable python symlink message Dec 31, 2020
@Rylan12
Copy link
Copy Markdown
Member Author

Rylan12 commented Dec 31, 2020

Okay, this PR now removed the check altogether.

In the future, we'll likely have to deal with the ubuntu-20.04 issue. Maybe I'll open a new PR and take a look to see if I can solve it.

@MikeMcQuaid
Copy link
Copy Markdown
Member

Maybe I'll open a new PR and take a look to see if I can solve it.

That'd be great, thanks ❤️. Happy to help if you get blocked.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 31, 2020
@BrewTestBot
Copy link
Copy Markdown
Contributor

Review period ended.

@Rylan12 Rylan12 merged commit 7e5237c into Homebrew:master Dec 31, 2020
@Rylan12 Rylan12 deleted the disable-python-doctor-message branch December 31, 2020 18:41
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 31, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

linux outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants