Skip to content

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.

@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

6 participants