Skip to content

Conversation

@lazka
Copy link
Collaborator

@lazka lazka commented Jul 30, 2019

Adds a job for Python 3.6/x86 and 3.7/x64 on Windows Server 2016.

@lazka
Copy link
Collaborator Author

lazka commented Jul 30, 2019

@vstinner
Copy link
Member

I fixed 2to3 benchmark differently:

commit 030c84ad26dd13391160e698dc18f82c08e9be5a (HEAD -> master, origin/master, origin/HEAD)
Author: Victor Stinner <vstinner@redhat.com>
Date:   Wed Jul 31 11:30:04 2019 +0200

    bm_2to3 now uses Runner.bench_command()

Can you please rebase on top of that?

@vstinner
Copy link
Member

I'm not sure that redirecting the output to "/dev/null" ("NUL" on Windows) has the same performance than using a pipe with Popen.communicate().

@vstinner
Copy link
Member

Note: bm_2to3.py used open(os.devnull, "wb") whereas pyperf uses open(os.devnull, 'w+', 0) (on Python 2). "w+" is needed because the same file is used to stdin and stdout.

Running with Python 2 on Windows required changes to the 2to3 benchmark
because it failed due to devnull being opened in binary mode.

What is the problem of the "b" in open(os.devnull, "wb")?

@lazka lazka force-pushed the ci-windows branch 2 times, most recently from 070aa29 to e29da37 Compare July 31, 2019 16:04
@lazka
Copy link
Collaborator Author

lazka commented Jul 31, 2019

Note: bm_2to3.py used open(os.devnull, "wb") whereas pyperf uses open(os.devnull, 'w+', 0) (on Python 2). "w+" is needed because the same file is used to stdin and stdout.

Running with Python 2 on Windows required changes to the 2to3 benchmark
because it failed due to devnull being opened in binary mode.

What is the problem of the "b" in open(os.devnull, "wb")?

Good question :/ I assumed some encoding issue and piping fixed it, but I tried locally now and I can't reproduce the error. The error though is still there in azure-pipelines with the rebase on top of your commit: https://dev.azure.com/lazka/pyperformance/_build/results?buildId=115 I'm a bit out of ideas.

Should I drop testing with Python 2?

Adds a job for Python 3.6/x86 and 3.7/x64 on Windows Server 2016.
@lazka
Copy link
Collaborator Author

lazka commented Jul 31, 2019

Should I drop testing with Python 2?

Gave up on Python 2 and added a py3.6/x86 job instead -> https://dev.azure.com/lazka/pyperformance/_build/results?buildId=127

@vstinner
Copy link
Member

vstinner commented Aug 1, 2019

Oh, maybe the bench_command() method pyperf.Runner is simply broken on Windows with Python 2? I rarely do benchmarks on Windows. I'm fine with only starting with Python 3, it's better than having no test on Windows.

Now I have another question: how can we get the CI integrated into GitHub? I don't see the Azure job in "All checks have passed" of this PR?

@vstinner
Copy link
Member

vstinner commented Aug 1, 2019

@lazka: I added (invited) you as a collaborator to the project, since you seems to be eager to fix pyperformance issues :-)

@lazka
Copy link
Collaborator Author

lazka commented Aug 1, 2019

Thanks! As far as I see I can only add a pipeline once it finds a config in a branch of the repo. And even then I'm not sure if it doesn't require setting up from a repo owner, but it doesn't show me in the wizard right now (appveyor is a lot more flexible there.. but slower)

I'll go ahead and merge this and try to register the repo with azure pipelines.

@lazka lazka merged commit 25c6f4e into python:master Aug 1, 2019
@lazka
Copy link
Collaborator Author

lazka commented Aug 1, 2019

Sadly it fails after trying to register it: Unable to configure a service on the selected GitHub repository. This is likely caused by not having the necessary permission to manage hooks for the selected repository.

So an owner of the repo needs to create a project on azure and add a build pipeline.

@lazka
Copy link
Collaborator Author

lazka commented Aug 1, 2019

@zooba can you register a CI pipeline on azure pipelines for this repo?

@vstinner
Copy link
Member

vstinner commented Aug 1, 2019

So an owner of the repo needs to create a project on azure and add a build pipeline.

How can I do that?

@lazka
Copy link
Collaborator Author

lazka commented Aug 2, 2019

https://www.azuredevopslabs.com/labs/azuredevops/github-integration

During "Choose template" there should be an option to use the existing .yaml file in the repo instead.

@lazka
Copy link
Collaborator Author

lazka commented Aug 9, 2019

I've pushed an AppVeyor config to master, in case that is easier for you.

@vstinner
Copy link
Member

FYI I still don't see any Windows CI on a new PR: #72 (if you click on Details, there is only Travis CI)

@lazka
Copy link
Collaborator Author

lazka commented Aug 12, 2019

I'm not a repo owner, so I can't connect it to appveyor. You'd have to do that.

@vstinner
Copy link
Member

I'm not a repo owner, so I can't connect it to appveyor. You'd have to do that.

I changed your permissions to "Admins". Would you mind to try to enable the AppVeyor integration?

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.

2 participants