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.

Sign up for GitHub

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

Jump to bottom

Attempt to use coverage to quantify how many of the CONDITIONAL_NAMES we really use #3763

Merged
merged 7 commits into from
Jul 9, 2017
Merged

Attempt to use coverage to quantify how many of the CONDITIONAL_NAMES we really use #3763

merged 7 commits into from
Jul 9, 2017

Conversation

alex
Copy link
Member

@alex alex commented Jul 8, 2017

The last time I sent this, we used it just as a temporary run. Now I want to propose we really use it :-)

@mention-bot
Copy link

@alex, thanks for your PR! By analyzing the history of the files in this pull request, we identified @reaperhulk, @public and @glyph to be potential reviewers.

@alex
Copy link
Member Author

alex commented Jul 8, 2017

@reaperhulk Cryptography_HAS_DTLS and Cryptography_HAS_EC2M are apparently unused in CI. Objections to removing them?

@reaperhulk
Copy link
Member

According to the changelog no-ec2m was only added in 1.1.0 so it seems unlikely that anybody actually uses it (why is it even an option...). No DTLS might be somewhere, but I guess the only way we're going to learn about it is to drop support and see what happens. So, 👍 on removing both.

@alex
Copy link
Member Author

alex commented Jul 8, 2017

I'll do seperate PRs 👍

alex added a commit to alex/cryptography that referenced this pull request Jul 8, 2017
@alex
Remove conditionals we never use.
9629ba9 
Refs pyca#3763
@alex alex mentioned this pull request Jul 8, 2017
Merged
reaperhulk pushed a commit that referenced this pull request Jul 8, 2017
@alex @reaperhulk
Remove conditionals we never use. (#3766)
538a953 
* Remove conditionals we never use.

Refs #3763

* put this back
@alex alex force-pushed the coverage-for-conditionals branch from 5d5e7a3 to 615d3ea Compare July 8, 2017 21:43
@alex
Copy link
Member Author

alex commented Jul 8, 2017

So, for whatever reason, codecov is not finding a coverage report on the CentOS7 builder, leading to this issue. wat.

@reaperhulk any clue why it fails?

@reaperhulk
Copy link
Member

Well that's bizarre. No idea what's going on. I guess we can pull down the image and run it ourselves to see what happens though.

reaperhulk
reaperhulk approved these changes Jul 9, 2017
View reviewed changes
Copy link
Member

@reaperhulk reaperhulk left a comment

Choose a reason for hiding this comment

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

Looks good. Let's remove EGD and cryptodev in an upcoming PR since we really shouldn't need or want them.

@reaperhulk reaperhulk merged commit 601ed63 into pyca:master Jul 9, 2017
@alex alex deleted the coverage-for-conditionals branch July 9, 2017 00:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@reaperhulk reaperhulk reaperhulk approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alex @mention-bot @reaperhulk