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

Switch from dotnet/msquic to microsoft/msquic #1020

Merged
merged 3 commits into from
May 14, 2024

Conversation

ManickaP
Copy link
Member

In order to archive https://github.com/dotnet/msquic we have to get rid of the references. This change replaces the dotnet repo with the official MsQuic one.

@CarnaViire
Copy link
Member

There seem to be build failures @ManickaP

@ManickaP ManickaP requested a review from a team as a code owner April 26, 2024 05:27
@ManickaP
Copy link
Member Author

Broken by: microsoft/msquic#3849
Fix in: microsoft/msquic#4264

@ManickaP ManickaP added the do-not-merge A PR that should not be merged yet but needs to remain open label Apr 26, 2024
@jkoritzinsky
Copy link
Member

Related to #990, we should move these docker images to use multi-stage images that build msquic in one stage and set up the deployed environment (with the built msquic) in another.

@CarnaViire
Copy link
Member

CarnaViire commented Apr 30, 2024

@jkoritzinsky The problem is that the build scripts (and therefore the produced binaries) are different, depending on the architecture (x64, arm, arm64) and openssl version (1.1, 3.0). So out of the 8 updated images here, 6 would have different MsQuic, so the environment cannot be reused between them.

Note that Alpine is the only platform where we don't have any official MsQuic packages (tracking issue dotnet/runtime#83789), so we have to resort to the manual build here. All the other images install official packages from packages.microsoft.com

@jkoritzinsky
Copy link
Member

I'm not saying we use a "builder" image or something like that to share between the images. I just mean that we do a second FROM statement and install the tools needed for runtime in the second stage (and copy msquic from the first stage) instead of having the build tools and msquic source on the image that runs in Helix.

@ManickaP ManickaP requested a review from wfurt May 13, 2024 08:35
@ManickaP
Copy link
Member Author

I'm not saying we use a "builder" image or something like that to share between the images. I just mean that we do a second FROM statement and install the tools needed for runtime in the second stage (and copy msquic from the first stage) instead of having the build tools and msquic source on the image that runs in Helix.

I'm not following this line of thought. And is it necessary for this PR or should it rather be resolved separately? After all, I'm just changing from where we take msquic sources, nothing else.

@ManickaP ManickaP requested review from CarnaViire and removed request for CarnaViire May 13, 2024 08:36
@ManickaP ManickaP removed the do-not-merge A PR that should not be merged yet but needs to remain open label May 13, 2024
@jkoritzinsky
Copy link
Member

It's not required for this PR. I can take a look at doing it after this is merged as an example.

@rzikm
Copy link
Member

rzikm commented May 14, 2024

Since we opt for consuming the released MsQuic versions, would it make sense to (always) consume the latest published packages where we can? (Ubuntu etc.)

The change does not have to be in this PR, but something to consider for the mentioned future improvements.

@ManickaP ManickaP merged commit b7e8df9 into dotnet:main May 14, 2024
29 checks passed
@ManickaP ManickaP deleted the msft-msquic branch May 14, 2024 12:17
@CarnaViire
Copy link
Member

Since we opt for consuming the released MsQuic versions, would it make sense to (always) consume the latest published packages where we can? (Ubuntu etc.)

@rzikm We do already:

apt-add-repository https://packages.microsoft.com/ubuntu/22.04/prod && \
apt-get update && \
apt-get install -y libmsquic && \

Alpine is the only one where we cannot install the official packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants