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

Dropping Python 2.7 host support #265

Merged
merged 7 commits into from
Feb 4, 2020
Merged

Conversation

YannickJadoul
Copy link
Member

See #245

@YannickJadoul YannickJadoul force-pushed the drop-2.7 branch 12 times, most recently from 43ceb09 to 91fffc4 Compare February 3, 2020 16:13
@YannickJadoul
Copy link
Member Author

YannickJadoul commented Feb 3, 2020

Can't seem to (easily & quickly) install a Python 3.5 on a macOS CI, but following @mayeut's example, see CI.md:

This is a summary of the Python versions and platforms covered by the different CI platforms:

3.5 3.6 3.7 3.8
Linux Travis CI CircleCI AppVeyor Azure Pipelines
macOS CircleCI Travis CI¹ / CircleCI Azure Pipelines
Windows TravisCI AppVeyor (32-bit) AppVeyor (64-bit) Azure Pipelines

¹ Python version not really pinned, but dependent on the (default) version of image used.

@YannickJadoul
Copy link
Member Author

@Czaki I remember there was a situation where subprocess.run would have been nice to use in one of your PRs, but it wasn't possible because of Python 2.7. Do you still remember where that was?

@Czaki
Copy link
Contributor

Czaki commented Feb 3, 2020

I need to check. If i good remember it is connected with checking if proper messages are produced in #156. But now this check are in unit_test part (or can be done here). You can check it or wart few hours.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Feb 3, 2020

If i good remember it is connected with checking if proper messages are produced in #156.

I think you are right. But I cannot immediately find it.

There's no hurry. And don't worry if you do not find it; it's not very important. Just wondering if you remembered.

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

appveyor.yml Outdated
matrix:
- image: Ubuntu
- image: Visual Studio 2015
PYTHON_VERSION: 36
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor (sorry!), PYTHON_DIR: C:\\Python36 might be clearer than this?

@YannickJadoul
Copy link
Member Author

Thanks, @joerick! All done, committed, and pushed, except for:

https://github.com/joerick/cibuildwheel/blob/master/setup.py#L1 (change to python3?)

I just removed the shebang line, there; it feels a bit weird for setup.py to have that, and StackOverflow seems to agree with me: https://stackoverflow.com/questions/35701131/why-does-setup-py-usually-not-have-a-shebang-line

test/07_ssl/setup.py Outdated Show resolved Hide resolved
@YannickJadoul
Copy link
Member Author

If you would have said something here, I could have quickly done it here, instead of with the PR on a PR, @Czaki...

@Czaki
Copy link
Contributor

Czaki commented Feb 3, 2020

It was easier to test then suggest change. But when i have code then it is easier to make PR than write. Unfortunatelly github does not allow to add comments to lines which are not changed...

I promise to suggest such simple changes as comment.

@YannickJadoul
Copy link
Member Author

Ah, I see :-) Sorry, then. But I would've found those (object) things also easily, if you had told me. That's what I meant.
Anyway, good catch!

@Czaki
Copy link
Contributor

Czaki commented Feb 3, 2020

When I starting I think that it will need more changes. But I agree that this change is prety simple.

@joerick joerick mentioned this pull request Feb 3, 2020
5 tasks
@joerick
Copy link
Contributor

joerick commented Feb 4, 2020

Python 3.5 doesn't have the "file system path protocol" (PEP 519) yet, meaning that you indeed need a bunch of str(...) calls when passing it as argument to e.g. subprocess or so. So I'll try to continue a bit to see how bad it is, but I expect it might not be worth it until we can drop 3.5.

I agree with this - the pathlib stuff will be nice, but only when we can use them as conveniently as string paths.

Just had another look over it, the rest looks good - thanks for the suggestions @Czaki. Merge when you're ready!

@joerick
Copy link
Contributor

joerick commented Feb 4, 2020

The Appveyor fail is... weird... it looks like the Ubuntu image is trying to run the Windows cmd lines, in contradiction with their docs. However, it's not happening on master so there must be something in this config that's triggering it...

@Czaki
Copy link
Contributor

Czaki commented Feb 4, 2020

Why adding step to appveyor? This one is not pararell so it will increase time with waiting on test result.

@YannickJadoul
Copy link
Member Author

Why adding step to appveyor? This one is not pararell so it will increase time with waiting on test result.

Because Windows wasn't covered that well, yet. And because I also thought it would be good to try a 64-bit and 32-bit test. Does it take so long? AppVeyor is always the slowest anyway, because it doesn't cancel old builds (although that can be configured).

@YannickJadoul
Copy link
Member Author

The Appveyor fail is... weird... it looks like the Ubuntu image is trying to run the Windows cmd lines, in contradiction with their docs. However, it's not happening on master so there must be something in this config that's triggering it...

I'm trying again. It used to work, because some build succeeded, before, I thought. Or maybe it just didn't fail yet.

@YannickJadoul
Copy link
Member Author

I'm trying again. It used to work, because some build succeeded, before, I thought. Or maybe it just didn't fail yet.

OK, maybe it never worked :-(

@Czaki
Copy link
Contributor

Czaki commented Feb 4, 2020

Maybe better is to add this test to azure pilenes? It has python 32-bits and parallelism work (10 workers)

I think that problem is with using matrix.

@YannickJadoul
Copy link
Member Author

@Czaki ^ See what I just did?

@YannickJadoul
Copy link
Member Author

@Czaki ^ See what I just did?

Didn't fancy debugging the AppVeyor config, again.

The proposed CI table now looks like this:

3.5 3.6 3.7 3.8
Linux Travis CI CircleCI AppVeyor Azure Pipelines
macOS Azure Pipelines CircleCI Travis CI¹ / CircleCI Azure Pipelines
Windows TravisCI Azure Pipelines AppVeyor Azure Pipelines

@joerick Do you agree with fully trying to cover those different versions, or are those extra two Azure builds too much?

@Czaki
Copy link
Contributor

Czaki commented Feb 4, 2020

You write about Windows 32 bits test. I do not see them anywhere. It can be done on azure pipelines with :

- task: UsePythonVersion@0
  inputs:
    versionSpec: '3.6' 
    #addToPath: true 
    architecture: 'x86' # Options: x86, x64 (this argument applies only on Windows agents)

https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/tool/use-python-version?view=azure-devops#yaml-snippet

@YannickJadoul
Copy link
Member Author

You write about Windows 32 bits test. I do not see them anywhere.

AppVeyor is 32-bit by default (https://www.appveyor.com/docs/windows-images-software/#python)
And actually, after your comment, I thought about it again, and I don't think the host Python version makes a big difference in 64- vs 32-bit?

@joerick
Copy link
Contributor

joerick commented Feb 4, 2020

I'm not concerned about host 32/64 bit tbh, that's only a concern on the build side.

As far as the matrix goes, i think it's good to cover all platforms on 3.5 and the latest, I'm not too worried about gaps in the middle. But where possible, it's good to spread them out and avoid duplicates.

@YannickJadoul
Copy link
Member Author

OK, then let's keep it like this. Azure is often fast and parallel enough, and if builds start being too slow, we can easily drop it again.

Let's also try to keep CI.md up to date. I think it's quite nice to have that overview somewhere? :-)

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.

3 participants