#7630 Adding better error messaging in mach bootstrap for missing virtualenv/pip dependencies #7678

Merged
merged 1 commit into from Sep 22, 2015

Conversation

Projects
None yet
5 participants
@AnthonyBroadCrawford

This PR is in reference to #7630

I've added a simple try catch around our use of subprocess.check_all when trying to invoke and use python's

  • virtualenv
  • pip

Upon failure, I use sys.exit with an error message for the user. Exit seemed appropriate as anything beneath those dependencies will fail to execute and result in a non friendly error message

Review on Reviewable

@highfive

This comment has been minimized.

Show comment
Hide comment
@highfive

highfive Sep 18, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 18, 2015

Member

So your current solution will indeed warn if the user doesn't have pip or virtualenv installed, though I'm a little worried an OSError will also get raised with some other issue during the os.check_call (network timeout, invalid permissions, corrupted files, etc). What do you think about this idea:

  1. Make _get_exec (the function in this the file) return None if both name and default return None when passed to find_executable.
  2. In _activate_virtualenv, call _get_exec for both virtualenv (this is already done) and pip and if the result is None, call sys.exit.
Member

frewsxcv commented Sep 18, 2015

So your current solution will indeed warn if the user doesn't have pip or virtualenv installed, though I'm a little worried an OSError will also get raised with some other issue during the os.check_call (network timeout, invalid permissions, corrupted files, etc). What do you think about this idea:

  1. Make _get_exec (the function in this the file) return None if both name and default return None when passed to find_executable.
  2. In _activate_virtualenv, call _get_exec for both virtualenv (this is already done) and pip and if the result is None, call sys.exit.
@AnthonyBroadCrawford

This comment has been minimized.

Show comment
Hide comment
@AnthonyBroadCrawford

AnthonyBroadCrawford Sep 18, 2015

@frewsxcv That makes sense, let me take a stab at it.

Additionally, are there unit tests for these scripts? I couldn't find any.

@frewsxcv That makes sense, let me take a stab at it.

Additionally, are there unit tests for these scripts? I couldn't find any.

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 18, 2015

Member

I am not aware of any unit tests for these bootstrapping scripts. That said, if you're considering adding some, I might recommend opening a dedicated GitHub Issue for it so others can chime in their opinions.

Member

frewsxcv commented Sep 18, 2015

I am not aware of any unit tests for these bootstrapping scripts. That said, if you're considering adding some, I might recommend opening a dedicated GitHub Issue for it so others can chime in their opinions.

@AnthonyBroadCrawford

This comment has been minimized.

Show comment
Hide comment
@AnthonyBroadCrawford

AnthonyBroadCrawford Sep 20, 2015

@frewsxcv I've made some changes to the code and pushed up to this branch.

I went down the path of expanding the scope on the method _get_exec as you suggested however, while digging into the documentation for subprocess.check_call I realized we are really dealing with three scenarios

  1. The referenced executable fails during execution
  2. The referenced executable is not found
  3. The user doesn't have permissions to execute the executable even when found

Digging into the documentation on subprocess.check_call I found that it will return two types of errors

  • subprocess.CalledProcessError when the executable fails during execution (use-case 1 mentioned above)
    -OSError When the process isn't found nor the user has permissions to execute the executable (use-case 2 && 3 from above)

Given that, thought it might be a less invasive change if we expand our exception handling and messaging for the use-cases around our use of check_call.

So, my proposed change is to

  1. Catch both exception types subprocess.CalledProcessError and OSError
  2. Provide more targeted user friendly error messaging to the user to help steer them to the appropriate resolution.

I'm still 100% open to going down the other path (and to be clear, I did) but it seemed like I was really expanding scope on those functions when this might be less invasive change that helps make the error messages more user friendly when these executables are not installed on their machine.

Thoughts?

@frewsxcv I've made some changes to the code and pushed up to this branch.

I went down the path of expanding the scope on the method _get_exec as you suggested however, while digging into the documentation for subprocess.check_call I realized we are really dealing with three scenarios

  1. The referenced executable fails during execution
  2. The referenced executable is not found
  3. The user doesn't have permissions to execute the executable even when found

Digging into the documentation on subprocess.check_call I found that it will return two types of errors

  • subprocess.CalledProcessError when the executable fails during execution (use-case 1 mentioned above)
    -OSError When the process isn't found nor the user has permissions to execute the executable (use-case 2 && 3 from above)

Given that, thought it might be a less invasive change if we expand our exception handling and messaging for the use-cases around our use of check_call.

So, my proposed change is to

  1. Catch both exception types subprocess.CalledProcessError and OSError
  2. Provide more targeted user friendly error messaging to the user to help steer them to the appropriate resolution.

I'm still 100% open to going down the other path (and to be clear, I did) but it seemed like I was really expanding scope on those functions when this might be less invasive change that helps make the error messages more user friendly when these executables are not installed on their machine.

Thoughts?

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 20, 2015

Member

python/mach_bootstrap.py, line 95 [r2] (raw file):
This shouldn't be indented, right? Otherwise, the virtual environment will only get activated when it doesn't exist


Comments from the review on Reviewable.io

Member

frewsxcv commented Sep 20, 2015

python/mach_bootstrap.py, line 95 [r2] (raw file):
This shouldn't be indented, right? Otherwise, the virtual environment will only get activated when it doesn't exist


Comments from the review on Reviewable.io

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 20, 2015

Member

That all seems alright to me. I left a comment on Reviewable before that needs to be addressed

Member

frewsxcv commented Sep 20, 2015

That all seems alright to me. I left a comment on Reviewable before that needs to be addressed

@AnthonyBroadCrawford

This comment has been minimized.

Show comment
Hide comment
@AnthonyBroadCrawford

AnthonyBroadCrawford Sep 21, 2015

@frewsxcv Fixed the indentation issue. My apologies, still refreshing myself on Python as it has been a while.

@frewsxcv Fixed the indentation issue. My apologies, still refreshing myself on Python as it has been a while.

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 21, 2015

Member

@AnthonyBroadCrawford Thanks!

It looks like there is a 'tidy' failure (check the Travis CI failure for more information)

./python/mach_bootstrap.py:93: Line is longer than 120 characters

You should be able to do something like this:

sys.exit("Python virtualenv not found. Please install virtualenv and ensure "
         "permissions prior to running mach.")
Member

frewsxcv commented Sep 21, 2015

@AnthonyBroadCrawford Thanks!

It looks like there is a 'tidy' failure (check the Travis CI failure for more information)

./python/mach_bootstrap.py:93: Line is longer than 120 characters

You should be able to do something like this:

sys.exit("Python virtualenv not found. Please install virtualenv and ensure "
         "permissions prior to running mach.")
@AnthonyBroadCrawford

This comment has been minimized.

Show comment
Hide comment
@AnthonyBroadCrawford

This comment has been minimized.

Show comment
Hide comment
@AnthonyBroadCrawford

AnthonyBroadCrawford Sep 21, 2015

@frewsxcv Fixed. Thanks for pointing that out.

@frewsxcv Fixed. Thanks for pointing that out.

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 21, 2015

Member

Looks great! 👍

Can you squash your commits into one? If you need help with this, let me know

Member

frewsxcv commented Sep 21, 2015

Looks great! 👍

Can you squash your commits into one? If you need help with this, let me know

Anthony Broad-Crawford Anthony Broad-Crawford
Added error handling and improved error messaging when running mach w…
…ithout python's virtualenv or pip installed
@AnthonyBroadCrawford

This comment has been minimized.

Show comment
Hide comment
@AnthonyBroadCrawford

AnthonyBroadCrawford Sep 21, 2015

@frewsxcv squashed down into a single commit.

@frewsxcv squashed down into a single commit.

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 21, 2015

Member

@AnthonyBroadCrawford Thanks for your contribution, great work!

@bors-servo r+

Member

frewsxcv commented Sep 21, 2015

@AnthonyBroadCrawford Thanks for your contribution, great work!

@bors-servo r+

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2015

Contributor

📌 Commit 2e0e228 has been approved by frewsxcv

Contributor

bors-servo commented Sep 21, 2015

📌 Commit 2e0e228 has been approved by frewsxcv

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2015

Contributor

⌛️ Testing commit 2e0e228 with merge a6208f0...

Contributor

bors-servo commented Sep 21, 2015

⌛️ Testing commit 2e0e228 with merge a6208f0...

bors-servo pushed a commit that referenced this pull request Sep 21, 2015

bors-servo
Auto merge of #7678 - AnthonyBroadCrawford:improved-error-messaging-m…
…ach-bootstrap, r=frewsxcv


#7630 Adding better error messaging in mach bootstrap for missing virtualenv/pip dependencies 

This PR is in reference to #7630 

I've added a simple try catch around our use of subprocess.check_all when trying to invoke and use python's 

- virtualenv 
- pip

Upon failure, I use sys.exit with an error message for the user.  Exit seemed appropriate as anything beneath those dependencies will fail to execute and result in a non friendly error message

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7678)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 21, 2015

Contributor

💔 Test failed - linux-dev

Contributor

bors-servo commented Sep 21, 2015

💔 Test failed - linux-dev

@AnthonyBroadCrawford

This comment has been minimized.

Show comment
Hide comment
@AnthonyBroadCrawford

AnthonyBroadCrawford Sep 21, 2015

@frewsxcv, @jdm hmmm, I'm not familiar with what I'm looking at in the "homu" log files. Pointing me in the correct direction would be super helpful.

@frewsxcv, @jdm hmmm, I'm not familiar with what I'm looking at in the "homu" log files. Pointing me in the correct direction would be super helpful.

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Sep 21, 2015

Member

@AnthonyBroadCrawford Don't worry, there's an issue with the builders right now. Once the issue is worked out, I'll tell the builders to retry your changes

Member

frewsxcv commented Sep 21, 2015

@AnthonyBroadCrawford Don't worry, there's an issue with the builders right now. Once the issue is worked out, I'll tell the builders to retry your changes

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2015

Contributor

⌛️ Testing commit 2e0e228 with merge 44de917...

Contributor

bors-servo commented Sep 22, 2015

⌛️ Testing commit 2e0e228 with merge 44de917...

bors-servo pushed a commit that referenced this pull request Sep 22, 2015

bors-servo
Auto merge of #7678 - AnthonyBroadCrawford:improved-error-messaging-m…
…ach-bootstrap, r=frewsxcv


#7630 Adding better error messaging in mach bootstrap for missing virtualenv/pip dependencies 

This PR is in reference to #7630 

I've added a simple try catch around our use of subprocess.check_all when trying to invoke and use python's 

- virtualenv 
- pip

Upon failure, I use sys.exit with an error message for the user.  Exit seemed appropriate as anything beneath those dependencies will fail to execute and result in a non friendly error message

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7678)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
Contributor

bors-servo commented Sep 22, 2015

@bors-servo bors-servo merged commit 2e0e228 into servo:master Sep 22, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment