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

Fix Travis runs with Python 3 #1843

Closed
wants to merge 5 commits into from

Conversation

joaocgreis
Copy link
Member

Checklist
Description of change

This fixes the remaining issues with test-find-python.js and running on Windows when using Python 3. This makes node-gyp compatible with Python 3 for everything that we run in CI (which is not that much).

@cclauss the fixes here seemed logical to me, but I don't have much experience with Python, so a careful review would be very welcome.

This PR is semver-patch, using Python 3 still depends on EXPERIMENTAL_NODE_GYP_PYTHON3. I am preparing a different PR to address that.

@joaocgreis
Copy link
Member Author

@joaocgreis joaocgreis mentioned this pull request Jul 23, 2019
2 tasks
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.

Very, very impressive! Thanks massively!

Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

lgtm.

@cclauss
Copy link
Contributor

cclauss commented Jul 24, 2019

It would be of interest to know if the Travis tests would pass on Python 3 on macOS. Please consider adding the changes from #1846 into this PR.

@cclauss cclauss requested a review from bnoordhuis July 24, 2019 15:59
t.strictEqual(stdout, '')
t.ok(/Python 2/.test(stderr))
t.ok(stdout === '' || stderr === '')
t.ok(/Python 2/.test(stderr) || /Python 3/.test(stdout))
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking but I think these two tests are going to be hard to debug if they fail (in the previous code if stdout wasn't equal to '' the contents would be included in the test failure).

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to refactor this so that if we're on Python 3, we run checks for that, which are strict and so output contents if they fail, otherwise we run stricts for Python 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, PTAL.

@joaocgreis
Copy link
Member Author

Updated to include the macOS Travis run as discussed in #1846.

@rvagg @MattIPv4 @richardlau let me know if you have any objection.

.travis.yml Outdated
@@ -44,6 +45,12 @@ matrix:
python: 3.7
env: NODE_GYP_FORCE_PYTHON=python3 EXPERIMENTAL_NODE_GYP_PYTHON3=1
before_install: nvm install 12
- name: "Python 3.7 on macOS"
os: osx
osx_image: xcode11
Copy link
Contributor

@cclauss cclauss Jul 25, 2019

Choose a reason for hiding this comment

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

I know that I am trying your patience but... can you please do me a favor and comment out line 50. The error messages (and investigations elsewhere) suggest to me that using the current xcode11 might be part of our problem.

https://docs.travis-ci.com/user/reference/osx/#xcode-version

+No receipt for 'com.apple.pkg.DeveloperToolsCLILeo' found at '/'.
    +No receipt for 'com.apple.pkg.DeveloperToolsCLI' found at '/'.
    +gyp: No Xcode or CLT version detected!

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem. Pushed a commit just to get a Travis run, I will discard it if xcode11 is not the problem.

The Python 2 version also uses xcode11, so I don't think this will fix it. If it does, we have to understand why it's not a problem with Python 2.

@cclauss
Copy link
Contributor

cclauss commented Jul 25, 2019

It moved us from 6 failing down to 3 failing. Thanks much!

This PR is a sold step forward.

@gibfahn Can you provide us any guidance on these three failures which appear when Travis CI uses the current osx_image: xcode11. This PR gets Linux and Windows passing our Travis CI tests on Python 3.7 so it would be great to add macOS to that list.

joaocgreis pushed a commit that referenced this pull request Jul 26, 2019
joaocgreis added a commit that referenced this pull request Jul 26, 2019
Fixes: #1826
PR-URL: #1843
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matt Cowley <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
joaocgreis added a commit that referenced this pull request Jul 26, 2019
PR-URL: #1843
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matt Cowley <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@joaocgreis
Copy link
Member Author

CI: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/153/ ✔️

Landed in 5459eca...2592036.

Thanks for the reviews!

@cclauss left the xcode line commented. Notice the number of passing tests is the same, so probably when a test fails we can't really consider the remaining tests in that file. xcode11 works with Python 2, so it may make sense to duplicate the Python 3 entry, with and without xcode.

For reference:

@joaocgreis joaocgreis closed this Jul 26, 2019
@cclauss
Copy link
Contributor

cclauss commented Jul 26, 2019

Prefect. Thanks for this work. Legacy Python continues to pass no matter what.

Will add tests for both over the weekend.

@rvagg rvagg mentioned this pull request Sep 26, 2019
rvagg pushed a commit that referenced this pull request Sep 26, 2019
rvagg pushed a commit that referenced this pull request Sep 26, 2019
Fixes: #1826
PR-URL: #1843
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matt Cowley <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
rvagg pushed a commit that referenced this pull request Sep 26, 2019
PR-URL: #1843
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matt Cowley <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@rvagg rvagg mentioned this pull request Sep 26, 2019
targos added a commit to targos/node that referenced this pull request Oct 9, 2019
targos added a commit to nodejs/node that referenced this pull request Oct 13, 2019
targos added a commit to nodejs/node that referenced this pull request Nov 8, 2019
targos added a commit to nodejs/node that referenced this pull request Nov 10, 2019
BaochengSu pushed a commit to BaochengSu/node that referenced this pull request Oct 20, 2020
Refs: nodejs/node-gyp#1820
Refs: nodejs/node-gyp#1843

PR-URL: nodejs#29897
Reviewed-By: Christian Clauss <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
(cherry picked from commit 66b9532)
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.

5 participants