Skip to content

Remove Support for quictls v1.1 #5031

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
Merged
merged 18 commits into from
Apr 23, 2025
Merged

Remove Support for quictls v1.1 #5031

merged 18 commits into from
Apr 23, 2025

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Apr 20, 2025 •
edited
Loading

Description

Since quictls/openssl v1.1 is no longer supported, remove support from msquic.

Testing

CI/CD

Documentation

Updated

@codecov Codecov
Copy link

codecov bot commented Apr 20, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.87%. Comparing base (a7c08fc) to head (15a708e).
Report is 5 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5031      +/-   ##
==========================================
+ Coverage   86.39%   86.87%   +0.47%     
==========================================
  Files          59       59              
  Lines       18011    18011              
==========================================
+ Hits        15561    15647      +86     
+ Misses       2450     2364      -86

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nibanks
Copy link
Member Author

nibanks commented Apr 21, 2025

@nhorman perhaps you could help me with this weird build issue we're seeing with Rust:

https://github.com/microsoft/msquic/actions/runs/14577495817/job/40886758174?pr=5031#step:16:29

Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.
  Added: /Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/html
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/html/man1
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/html/man3
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/html/man5
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/html/man7
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/man
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/man/man1
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/man/man3
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/man/man5
  	/Users/runner/work/msquic/msquic/target/package/msquic-2.5.0-beta/submodules/quictls/doc/man/man7

For some reason when we build Rust against quictls v3 it seems to be changing files under the doc subdirectory. Any ideas why? Rust doesn't like that it's now "dirty".

@nhorman
Copy link
Contributor

nhorman commented Apr 21, 2025

Just an initial guess, but my first thought would be the fact that openssl generates the files that you are hitting errors on using perldoc, and so perhaps those files are getting left over from the build?

Its odd, as I would have thought anything in this area would have been hit in quictls1.1 (unless that tree did something internally to not build docs).

If this hypothesis is vaguely accurate, I would think that the fast solution would be to configure openssl in submodules/CMakeLists.txt such that OPENSSL_CONFIG_FLAGS included the no-docs option. That would prevent any doc building, and given that (I think) you don't actually package the openssl docs, speed up the build somewhat considerably).

@nibanks
Copy link
Member Author

nibanks commented Apr 21, 2025

Just an initial guess, but my first thought would be the fact that openssl generates the files that you are hitting errors on using perldoc, and so perhaps those files are getting left over from the build?

Its odd, as I would have thought anything in this area would have been hit in quictls1.1 (unless that tree did something internally to not build docs).

If this hypothesis is vaguely accurate, I would think that the fast solution would be to configure openssl in submodules/CMakeLists.txt such that OPENSSL_CONFIG_FLAGS included the no-docs option. That would prevent any doc building, and given that (I think) you don't actually package the openssl docs, speed up the build somewhat considerably).

Ok. Thanks! I'll try the no-docs option. I had to add the docs folder to get quictls v3 to build. It wasn't there (nor seemed to be required) for v1.1....

@nhorman
Copy link
Contributor

nhorman commented Apr 21, 2025

that would support the theory that either openssl1.1 separated out the doc build somehow, or quictls modified the build process.

@nhorman
Copy link
Contributor

nhorman commented Apr 21, 2025

grrr, quictls3 doesn't appear to support no-docs. Let me look into that

@nibanks
Copy link
Member Author

nibanks commented Apr 21, 2025

grrr, quictls3 doesn't appear to support no-docs. Let me look into that

Thanks. I'm trying to see what its replacement might be... but I don't know the openssl/quictls code well

@nhorman
Copy link
Contributor

nhorman commented Apr 21, 2025

damn, looks like the no-docs option was added in openssl-3.2.0, so the build for quictls is always going to produce docs.

Next best option (I think) would be to, after the build is complete for quictls, cd into docs, and run git clean. Ugly, but it should work.

@nibanks nibanks marked this pull request as ready for review April 22, 2025 11:03
@nibanks nibanks requested a review from a team as a code owner April 22, 2025 11:03
anrossi
anrossi reviewed Apr 22, 2025
View reviewed changes
Copy link
Contributor

@anrossi anrossi left a comment

Choose a reason for hiding this comment

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

I see some changes to the packaging automation; does the CI run this as well so we know if this breaks things?

@nibanks
Copy link
Member Author

nibanks commented Apr 22, 2025

I see some changes to the packaging automation; does the CI run this as well so we know if this breaks things?

If it's in GitHub, then it runs it all; otherwise we will have to merge it and see what breaks.

@anrossi
Copy link
Contributor

anrossi commented Apr 23, 2025

Are these ubuntu test failures unrelated? I know the babeltrace crash is unrelated, but the test failure is the same on two of the test runs. I started a rerun on one of them

@nibanks
Copy link
Member Author

nibanks commented Apr 23, 2025

Are these ubuntu test failures unrelated? I know the babeltrace crash is unrelated, but the test failure is the same on two of the test runs. I started a rerun on one of them

Yes, they are unrelated.

@nibanks nibanks merged commit 12ae902 into main Apr 23, 2025
280 of 286 checks passed
@nibanks nibanks deleted the remove-quictls1 branch April 23, 2025 10:45
@nibanks nibanks self-assigned this Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anrossi anrossi anrossi approved these changes

Assignees

@nibanks nibanks

Labels
TLS: OpenSSL
Projects
DPT Iteration Tracker
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nibanks @nhorman @anrossi