GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43811: Test multiple OpenSSL versions on GHA by tiran · Pull Request #25360 · python/cpython

Conversation

@tiran

@tiran tiran commented Apr 12, 2021 •
edited by miss-islington
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The new checks are only executed when one or more OpenSSL-related files are modified. The checks run a handful of networking and hashing test suites. All SSL checks are optional. This PR also introduces ccache to speed up compilation. In common cases it speeds up configure and compile time from about 90 seconds to less than 30 seconds.

Signed-off-by: Christian Heimes [email protected]

https://bugs.python.org/issue43811

Automerge-Triggered-By: GH:tiran

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tiran tiran added needs backport to 3.8 labels Apr 12, 2021
@tiran
tiran requested review from vstinner and zooba April 12, 2021 10:02
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 12, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Apr 12, 2021
@tiran

tiran commented Apr 12, 2021

Copy link
Copy Markdown
Member Author

#25361 addresses the unrelated test failures.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran
tiran force-pushed the bpo-43811-gha-multissl branch from a741341 to 302c398 Compare April 12, 2021 11:13
vstinner
vstinner reviewed Apr 12, 2021
View reviewed changes
Comment thread .github/workflows/build.yml Outdated

@vstinner vstinner Apr 12, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Don't we have an accepted PEP which requires OpenSSL 1.1.1 or newer?

Each additional CI increases the error rate since we still have unstable tests in asyncio and multiprocessing tests. I would prefer to not add too many CIs. At least, don't make them mandatory (allow to merge a PR even if they fail).

Rather than pre-commit CI jobs run on every since PR and at every PR update, can't we use a post-commit CI, like a buildbot worker? Issues in pre-commit are way more annoying.

cc @pablogsal @zware

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tiran tiran Apr 12, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Don't we have an accept PEP which requires OpenSSL 1.1.1 or newer?

Yes, but the PEP applies to 3.10 only. You want OpenSSL 3.0.0 compatibility in 3.8 and 3.9, don't you?

Rather than pre-commit CI jobs run on every since PR and at every PR update, can't we use a post-commit CI, like a buildbot worker? Issues in pre-commit are way more annoying.

I need an easy and reliable way to ensure that Python works with all supported OpenSSL versions. Post-commit CI is too late.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner Apr 12, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can add OpenSSL < 1.1.1 in the CI jobs when you backport this PR to 3.9 and older. But this PR targets master, so I expect not see OpenSSL < 1.1.1 in this branch.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tiran tiran Apr 12, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Master still supports OpenSSL 1.1.0 and 1.0.2. I have not finished and pushed the PEP 644 PR. I deliberately postponed PEP 644 changes until OpenSSL 3.0.0 support is done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tiran tiran Apr 12, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Each additional CI increases the error rate since we still have unstable tests in asyncio and multiprocessing tests. I would prefer to not add too many CIs. At least, don't make them mandatory (allow to merge a PR even if they fail).

The additional SSL tests are now only executed when any OpenSSL or GHA related file is modified. The SSL tests only execute a couple of selected tests. Multiprocessing tests are skipped. The CI steps are not mandatory.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner Apr 13, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"Master still supports OpenSSL 1.1.0 and 1.0.2. I have not finished and pushed the PEP 644 PR. I deliberately postponed PEP 644 changes until OpenSSL 3.0.0 support is done."

Ok.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
vstinner
vstinner reviewed Apr 12, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please don't add a new CI until all issues are fixed, like this one:

"GitHub Actions / Ubuntu SSL tests with OpenSSL 3.0.0-alpha14
‘ERR_func_error_string’ is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tiran

tiran commented Apr 12, 2021

Copy link
Copy Markdown
Member Author

Please don't add a new CI until all issues are fixed, like this one:

"GitHub Actions / Ubuntu SSL tests with OpenSSL 3.0.0-alpha14
‘ERR_func_error_string’ is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]"

This is a chicken and egg problem. #25329 needs this PR to verify that the code works.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Apr 12, 2021

Copy link
Copy Markdown
Member

This is a chicken and egg problem. #25329 needs this PR to verify that the code works.

I'm spending a lot of time to fix every single warning in Python. I would prefer to not add new warnings, especially if you know that a lot of will be needed to fix them.

Can't you run this CI job locally, fix warnings, and then automate checks on PRs?

I would prefer not annoy contributors who don't work on OpenSSL support with these new deprecation warnings.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 12, 2021

Copy link
Copy Markdown
Member Author

This is a chicken and egg problem. #25329 needs this PR to verify that the code works.

I'm spending a lot of time to fix every single warning in Python. I would prefer to not add new warnings, especially if you know that a lot of will be needed to fix them.

Can't you run this CI job locally, fix warnings, and then automate checks on PRs?

I would prefer not annoy contributors who don't work on OpenSSL support with these new deprecation warnings.

I prefer to add CI first, so I have a stable baseline. Then I can fix deprecation warnings.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran

tiran commented Apr 12, 2021

Copy link
Copy Markdown
Member Author

I have added a new check_source instruction. The additional OpenSSL tests are skipped unless the commit touches any file with ssl, hmac, hashlib or ^.github in the file name.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tiran added 2 commits April 13, 2021 08:45
@tiran
bpo-43811: Test multiple OpenSSL versions on GHA
771ad62
Signed-off-by: Christian Heimes <[email protected]>
@tiran
Only run ssl tests when OpenSSL related files are changed
3669c9f
@tiran
tiran force-pushed the bpo-43811-gha-multissl branch from 7e18dda to 3669c9f Compare April 13, 2021 06:46
@tiran

tiran commented Apr 13, 2021

Copy link
Copy Markdown
Member Author

@vstinner @pablogsal I have reworked the PR. Additional checks are optional and only executed when GHA detects a modification of an OpenSSL-related file or a file in .github directory. I'm postponing PEP 644 PR until I have finalized OpenSSL 3.0.0 support. It should be ready by next week.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran
tiran requested a review from vstinner April 13, 2021 16:26
vstinner
vstinner approved these changes Apr 13, 2021
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Additional checks are optional and only executed when GHA detects a modification of an OpenSSL-related file

Ok, I'm fine with this PR in this case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tiran reacted with heart emoji
All reactions
  • ❤️ 1 reaction
Comment thread .github/workflows/build.yml Outdated

@vstinner vstinner Apr 13, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"Master still supports OpenSSL 1.1.0 and 1.0.2. I have not finished and pushed the PEP 644 PR. I deliberately postponed PEP 644 changes until OpenSSL 3.0.0 support is done."

Ok.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Apr 13, 2021
@tiran tiran added the 🤖 automerge label Apr 13, 2021
@miss-islington
miss-islington merged commit 8fa1489 into python:master Apr 13, 2021
@miss-islington

miss-islington commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Apr 13, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 13, 2021
@tiran @miss-islington
bpo-43811: Test multiple OpenSSL versions on GHA (pythonGH-25360)
8e228c5
The new checks are only executed when one or more OpenSSL-related files are modified. The checks run a handful of networking and hashing test suites. All SSL checks are optional. This PR also introduces ccache to speed up compilation. In common cases it speeds up configure and compile time from about 90 seconds to less than 30 seconds.

Signed-off-by: Christian Heimes <[email protected]>
(cherry picked from commit 8fa1489)

Co-authored-by: Christian Heimes <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Apr 13, 2021
@bedevere-bot

bedevere-bot commented Apr 13, 2021

Copy link
Copy Markdown

GH-25391 is a backport of this pull request to the 3.9 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 13, 2021
@tiran @miss-islington
bpo-43811: Test multiple OpenSSL versions on GHA (pythonGH-25360)
65655ca
The new checks are only executed when one or more OpenSSL-related files are modified. The checks run a handful of networking and hashing test suites. All SSL checks are optional. This PR also introduces ccache to speed up compilation. In common cases it speeds up configure and compile time from about 90 seconds to less than 30 seconds.

Signed-off-by: Christian Heimes <[email protected]>
(cherry picked from commit 8fa1489)

Co-authored-by: Christian Heimes <[email protected]>
@bedevere-bot

bedevere-bot commented Apr 13, 2021

Copy link
Copy Markdown

GH-25392 is a backport of this pull request to the 3.8 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.8 label Apr 13, 2021
miss-islington added a commit that referenced this pull request Apr 13, 2021
@miss-islington
[3.9] bpo-43811: Test multiple OpenSSL versions on GHA (GH-25360) (GH…
5ce227f
…-25391)

The new checks are only executed when one or more OpenSSL-related files are modified. The checks run a handful of networking and hashing test suites. All SSL checks are optional. This PR also introduces ccache to speed up compilation. In common cases it speeds up configure and compile time from about 90 seconds to less than 30 seconds.

Signed-off-by: Christian Heimes <[email protected]>
(cherry picked from commit 8fa1489)


Co-authored-by: Christian Heimes <[email protected]>

Automerge-Triggered-By: GH:tiran
miss-islington added a commit that referenced this pull request Apr 13, 2021
@miss-islington
[3.8] bpo-43811: Test multiple OpenSSL versions on GHA (GH-25360) (GH…
a607815
…-25392)

The new checks are only executed when one or more OpenSSL-related files are modified. The checks run a handful of networking and hashing test suites. All SSL checks are optional. This PR also introduces ccache to speed up compilation. In common cases it speeds up configure and compile time from about 90 seconds to less than 30 seconds.

Signed-off-by: Christian Heimes <[email protected]>
(cherry picked from commit 8fa1489)


Co-authored-by: Christian Heimes <[email protected]>

Automerge-Triggered-By: GH:tiran
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@zooba zooba Awaiting requested review from zooba

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@tiran @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.