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

Bump geventhttpclient and switch back to use its original repo + fix windows issue with resource module #1388

Conversation

cyberw
Copy link
Collaborator

@cyberw cyberw commented May 22, 2020

... instead of its fork, now that there is a new release (I can trigger new releases later on as well). Relates to #1116

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #1388 into master will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1388      +/-   ##
==========================================
- Coverage   81.11%   81.07%   -0.05%     
==========================================
  Files          26       26              
  Lines        2335     2335              
  Branches      361      361              
==========================================
- Hits         1894     1893       -1     
- Misses        348      349       +1     
  Partials       93       93              
Impacted Files Coverage Δ
locust/main.py 19.02% <0.00%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23eb5e0...0140776. Read the comment docs.

@heyman
Copy link
Member

heyman commented May 22, 2020

Before we can merge this we need to update the geventhttpclient repo and setup building + uploading of wheels for both Linux, Mac and Windows. For Windows I think we need to use Appveyor. This has already been done for the forked repo (https://github.com/locustio/geventhttpclient), so we should be able to take the changes from there. If we merge this without binary wheels available for common platforms it would cause installation of Locust to fail for people who don't have a build environment set up with the necessary lib dependencies installed.

@cyberw
Copy link
Collaborator Author

cyberw commented May 22, 2020

I’ll have a look

@cyberw
Copy link
Collaborator Author

cyberw commented May 23, 2020

Hmm... windows packages are not uploaded (e.g look at 1.4.2 on pypi)

if ($env:APPVEYOR_REPO_TAG -eq "true") {
python -m pip install twine
python -m twine upload --skip-existing (resolve-path wheelhouse*.whl)
}

My guess is that APPVEYOR_REPO_TAG is not true, but I cant understand why. It may be related to the fact that builds are not triggered automatically (I have to do it manually)

@heyman
Copy link
Member

heyman commented May 24, 2020

My guess is that APPVEYOR_REPO_TAG is not true, but I cant understand why. It may be related to the fact that builds are not triggered automatically (I have to do it manually)

That sounds plausible. Are you lacking permissions to set up the triggers? If so I guess @gwik has to set it up or give us admin access. I guess an alternative could be to see if he'd be okay with moving the official geventhttpclient repo over to the locustio org?

@gwik
Copy link

gwik commented May 24, 2020

As I said before I have no issue moving the repo wherever there are people willing to maintain it.

@gwik
Copy link

gwik commented May 24, 2020

Just let me know what you need.

@cyberw
Copy link
Collaborator Author

cyberw commented May 24, 2020

I think it it least disruptive to keep it where it is (but moving it is also fine I guess). I guess you have given us all the permissions you can with the repo itself @gwik ? Maybe appveyor needs to be added to the project level?

@cyberw
Copy link
Collaborator Author

cyberw commented May 24, 2020

I changed the appveyor script to always try uploading (but set the settings to only trigger on tagged builds, should we manage to fix the trigger)

So we now have a good (but untested :) 1.4.2 package!

I still need to push a button on appveyor to get the windows wheel out, but that is ok for now.

…d of fork, now that there is a new release (I can trigger new releases later on as well). Relates to #1116
@cyberw cyberw force-pushed the bump-geventhttpclient-and-switch-back-to-use-original-repo-instead-of-fork branch from d615b59 to 19ec1cd Compare May 25, 2020 07:19
@cyberw cyberw merged commit a207489 into master May 25, 2020
@cyberw
Copy link
Collaborator Author

cyberw commented May 25, 2020

Tested on windows and it works (found a bug while I was at it). Merging this, but we can continue discussing here if you want :)

@cyberw cyberw deleted the bump-geventhttpclient-and-switch-back-to-use-original-repo-instead-of-fork branch May 25, 2020 07:32
@cyberw cyberw changed the title Bump geventhttpclient and switch back to use its original repo Bump geventhttpclient and switch back to use its original repo + fix windows issue May 25, 2020
@cyberw cyberw changed the title Bump geventhttpclient and switch back to use its original repo + fix windows issue Bump geventhttpclient and switch back to use its original repo + fix windows issue with resource module May 25, 2020
@heyman
Copy link
Member

heyman commented May 25, 2020

Tested on windows and it works (found a bug while I was at it). Merging this, but we can continue discussing here if you want :)

Great work!

We should still try to get automatic triggering to work I think so that windows releases aren't forgotten a few years down the line :).

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