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

Travis CI: macOS does not include Python anymore #1979

Closed
wants to merge 3 commits into from
Closed

Travis CI: macOS does not include Python anymore #1979

wants to merge 3 commits into from

Conversation

imatlopez
Copy link
Contributor

Checklist
Description of change

macOS stopped bundling python, this attempts to bring back travis tests on macOS

@cclauss
Copy link
Contributor

cclauss commented Nov 28, 2019

Thanks for doing this but please use pyenv install Python on Travis CI. It is way faster than brew and supports all Python versions including Python 3.8 which brew is still struggling to support.

Try pyenv install --list to see all the versions available.

@cclauss cclauss changed the title build: macOS does not include python anymore build: macOS does not include Python anymore Nov 28, 2019
@cclauss
Copy link
Contributor

cclauss commented Nov 28, 2019

Travis is dropping matrix: for jobs: so let's lose the matrix around line 4...

addons:
  homebrew:
    update: true
    packages:
      - npm
      - pyenv
jobs:

matrix:

This will ensure that all macOS jobs have access to the latest npm and pyenv.

.travis.yml Outdated Show resolved Hide resolved
@imatlopez
Copy link
Contributor Author

imatlopez commented Nov 28, 2019

@cclauss got it working, cleaning up. For pyenv to work the PATH needs to be changed and for python modules to work they need the python -m prefix

(there is no better way to test this is there?)

@imatlopez imatlopez changed the title build: macOS does not include Python anymore Travis CI: macOS does not include Python anymore Nov 28, 2019
@imatlopez
Copy link
Contributor Author

@cclauss ready for review :)

@rvagg
Copy link
Member

rvagg commented Nov 29, 2019

I've force pushed out the HEAD commit on master, c392a17, so it's in the list of commits here too to be reviewed, edited (needs test: as the prefix) and have metadata attached.

@rvagg rvagg mentioned this pull request Nov 29, 2019
4 tasks
cclauss and others added 3 commits November 28, 2019 20:23
Uses `pyenv` to manage MacOS python versions since its not included
in the environment.
Isolates the `MacOS` builds to test changes faster. Reorders Travis
builds by OS. Replaces `pyenv global` calls with properly set `PATH`
and `PYENV_VERSION` env vars. Does not assume python modules are in
the `PATH` so all python modules are prefixed with `python -m`.
Undoes the test isolation and removes `pyenv init` because it is
not needed.
@imatlopez
Copy link
Contributor Author

I've force pushed out the HEAD commit on master, c392a17, so it's in the list of commits here too to be reviewed, edited (needs test: as the prefix) and have metadata attached.

reworded commit names, is this what you meant?

@rvagg
Copy link
Member

rvagg commented Nov 29, 2019

reworded commit names, is this what you meant?

No, sorry, it's not your commit, we'll deal with it when merged but if you rebase onto master then you'll be without it. I think it comes from work @cclauss was doing for #1980 but got pushed to master prematurely?

@imatlopez
Copy link
Contributor Author

Do you know why the python 3.5 test is failing? Can it be removed?

@rvagg
Copy link
Member

rvagg commented Nov 29, 2019

nope, will wait for @cclauss' input on that

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! Well done.

@rvagg
Copy link
Member

rvagg commented Nov 29, 2019

@cclauss got any suggestions on the 3.5 failure here?

@cclauss
Copy link
Contributor

cclauss commented Nov 29, 2019

On the 3.5 tests, those seem to be diff tests and my sense is that all the same data is there but not necessarily in the right order. Perhaps those tests need to look at smaller chunks or sort things before looking at them.

I will study that output in the next few hours...

Update: The diff contains:

E -                'AdditionalOptions': '/more /J',
E ?                                           ---
E +                'AdditionalOptions': '/more',

So the /J switch has been removed in both tests.

@imatlopez
Copy link
Contributor Author

What is that flag for? Is it related to using python -m pytest vs pytest? I know one difference is that the first one sets the current working directory as the PYTHONPATH while the second one does not.

@joaocgreis
Copy link
Member

The 3.5 failure is not related to this PR, so I think this should land when ready.

I saw the 3.5 failure when working on #1961, but it went away on a second run. That points to something like a difference in Travis runners or a broken cache.

@targos (since I saw you enabled Travis here, but other org owners can also do this) can you clear the Travis cache? Thanks!

@targos
Copy link
Member

targos commented Dec 2, 2019

@joaocgreis sure, what should I remove (I don't know anything about Travis caches):

image

@joaocgreis
Copy link
Member

@targos please delete all repository caches (top right corner). Thanks!

I believe nodejs/node uses the cache to actually make the build possible because of timing issues, but here it should be ok to just delete everything.

@targos
Copy link
Member

targos commented Dec 2, 2019

@joaocgreis it's done

@richardlau
Copy link
Member

I restarted https://travis-ci.com/nodejs/node-gyp/jobs/261487549 after the cache was cleared but it still failed.

@imatlopez
Copy link
Contributor Author

@rvagg anything pending for merging? I saw #1982 and it looks promising but I don't understand the distinction between travis and gh actions.

rvagg pushed a commit that referenced this pull request Dec 15, 2019
Uses `pyenv` to manage MacOS python versions since its not included
in the environment.

rvagg: landing this from #1979 even though it wasn't from the original
author. Treating approval there as approval of this commit too.

PR-URL: #1979
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Dec 15, 2019
Reorder Travis builds by OS. Replace `pyenv global` calls with
properly set `PATH` and `PYENV_VERSION` env vars. Does not
assume python modules are in the `PATH` so all python
modules are prefixed with `python -m`.

PR-URL: #1979
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Dec 15, 2019

d6a7e0e - @cclauss' commit
345c70e - the rest, I had a bit of trouble coming up with a summary but I think I got the gist if ot in there

@rvagg rvagg closed this Dec 15, 2019
@rvagg
Copy link
Member

rvagg commented Dec 15, 2019

still dealing with linux 3.5 failures https://travis-ci.com/nodejs/node-gyp/builds/141142728, fwiw

@cclauss
Copy link
Contributor

cclauss commented Dec 16, 2019

See #1995 for more on Python 3.5 on Linux failures...

rvagg pushed a commit that referenced this pull request Jan 3, 2020
Uses `pyenv` to manage MacOS python versions since its not included
in the environment.

rvagg: landing this from #1979 even though it wasn't from the original
author. Treating approval there as approval of this commit too.

PR-URL: #1979
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Jan 3, 2020
Reorder Travis builds by OS. Replace `pyenv global` calls with
properly set `PATH` and `PYENV_VERSION` env vars. Does not
assume python modules are in the `PATH` so all python
modules are prefixed with `python -m`.

PR-URL: #1979
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants