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

Fix broken build on Alpine #4264

Merged
merged 7 commits into from May 14, 2024
Merged

Fix broken build on Alpine #4264

merged 7 commits into from May 14, 2024

Conversation

ManickaP
Copy link
Member

lsb-release is not present on Alpine, whereas os-release is on most distributions.

@ManickaP ManickaP requested a review from a team as a code owner April 26, 2024 06:48
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.51%. Comparing base (d677687) to head (1b43d88).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4264      +/-   ##
==========================================
- Coverage   85.77%   83.51%   -2.27%     
==========================================
  Files          56       56              
  Lines       15382    15382              
==========================================
- Hits        13194    12846     -348     
- Misses       2188     2536     +348     

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

@ManickaP
Copy link
Member Author

This is still not enough. Since the build kind of assumes Ubuntu as the only distro and enables XDP everywhere except Ubuntu 20.04:

set(LINUX_XDP_ENABLED TRUE)

Which leads to:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
BPF_LIB
    linked by target "platform" in directory /tmp/msquic/src/platform
NL_LIB
    linked by target "platform" in directory /tmp/msquic/src/platform
NL_ROUTE_LIB
    linked by target "platform" in directory /tmp/msquic/src/platform
XDP_LIB
    linked by target "platform" in directory /tmp/msquic/src/platform

on Alpine.

src/platform/CMakeLists.txt Outdated Show resolved Hide resolved
@ManickaP ManickaP force-pushed the ManickaP-patch-1 branch 2 times, most recently from 4dcaa92 to 146d3a0 Compare April 26, 2024 09:54
@ami-GS
Copy link
Contributor

ami-GS commented Apr 26, 2024

Thanks for the change. As far as I remember, our docker environment doesn't have /etc/os-release. let me double check

@ami-GS
Copy link
Contributor

ami-GS commented Apr 26, 2024

Exists!😵‍💫

scripts/build.ps1 Outdated Show resolved Hide resolved
scripts/build.ps1 Outdated Show resolved Hide resolved
@ManickaP ManickaP force-pushed the ManickaP-patch-1 branch 2 times, most recently from 89d05b0 to aff5c5f Compare May 13, 2024 10:48
ManickaP and others added 5 commits May 13, 2024 12:49
lsb-release is not present on Alpine, whereas os-release is on most distributions.
@ManickaP ManickaP requested review from ami-GS and nibanks May 13, 2024 11:22
@ManickaP
Copy link
Member Author

I renamed the parameter, updated the pipeline (hopefully in the right place) and removed QUIC_CARGO_BUILD, PTAL.

@nibanks
Copy link
Member

nibanks commented May 13, 2024

Everything looks good to me, but @ami-GS can you please take a look? I noticed netperf failed, but I wonder if this is because of something else being broken? I know you're working on that in a parallel PR.

nibanks
nibanks previously approved these changes May 13, 2024
@nibanks
Copy link
Member

nibanks commented May 13, 2024

What should we do for the Official, OneBranch builds? Enabled or disabled?

@ami-GS
Copy link
Contributor

ami-GS commented May 13, 2024

rerunning BVT xdp test...

@ami-GS
Copy link
Contributor

ami-GS commented May 13, 2024

Could you update Linux XDP documentation? I believe build.ps1 requires additional parameter

https://github.com/microsoft/msquic/blob/main/docs/BUILD.md#linux-xdp

wfurt
wfurt previously approved these changes May 13, 2024
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@ManickaP ManickaP dismissed stale reviews from wfurt and nibanks via 2048332 May 14, 2024 07:49
nibanks
nibanks previously approved these changes May 14, 2024
@ManickaP
Copy link
Member Author

BTW, I don't have rights to merge this, so unless there is more feedback to address (let me know if there still something is), feel free to merge on my behalf. And thanks all for help with this!

docs/BUILD.md Outdated Show resolved Hide resolved
docs/BUILD.md Outdated Show resolved Hide resolved
docs/BUILD.md Outdated Show resolved Hide resolved
@nibanks nibanks merged commit 0576776 into main May 14, 2024
351 of 356 checks passed
@nibanks nibanks deleted the ManickaP-patch-1 branch May 14, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants