-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Ensure that MPL_REPO_DIR is set on Travis #4158
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
Conversation
this is needed to run pep8 on examples. Fall out from the merge of matplotlib#4151
Ensure that MPL_REPO_DIR is set on Travis
My previous fix for this (#4151) was to use TRAVIS_BUILD_DIR which is already defined and is the same as MPL_REPO_DIR. Now we have a mix of MPL_REPO_DIR and TRAVIS_BUILD_DIR. I still think using TRAVIS_BUILD_DIR is clearer. |
@matthew-brett I think you misunderstand. We need MPL_REPO_DIR because the pep8 tests need to know where the examples, which are not installed, are. You never changed the PEP8 test to read from TRAVIS_BUILD_DIR thus the merge of your pull request resulted in the pep8 job on the examples being marked as known fail. This sets the variable only for the purpose of running the pep8 test correctly. Which is also what the comment on the same line tells you. The original issue was that MPL_REPO_DIR was used for other stuff where it was not intended. |
And IMHO special casing the PEP8 test to try reading TRAVIS_BUILD_DIR would be significantly less clear than defining this extra variable in the travis file |
Just to be clear - you agree that the (already defined by travis) TRAVIS_BUILD_DIR is the matplotlib source directory and is therefore equal to MPL_REPO_DIR? |
Ah sorry - I see - sorry - the matplotlib code is using MPL_REPO_DIR - that makes sense. |
And so I'd have a vague preference for only using MPL_REPO_DIR in the pep8 section to be clear that it is specifically needed for that section, and using TRAVIS_BUILD_DIR for the travis stuff ( |
This change just sets the variable it still uses TRAVIS_BUILD_DIR to do the navigation. There is no specific pep8 section that is just controlled by another env variable but we could wrap setting MPL_REPO_DIR in a if TEST_ARGS=--pep8 section to make it more clear. |
Sorry - you're completely right - no quarrel at all about this change - I was confused by an earlier pull request. |
When I merged #4151 I missed the fact that MPL_REPO_DIR is also used to find the examples when running the pep8 tests. This ensures that it is set correctly.