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

msquic depends on libnuma but packages don't state that #3421

Closed
1 of 4 tasks
wfurt opened this issue Feb 11, 2023 · 9 comments · Fixed by #3670
Closed
1 of 4 tasks

msquic depends on libnuma but packages don't state that #3421

wfurt opened this issue Feb 11, 2023 · 9 comments · Fixed by #3670

Comments

@wfurt
Copy link
Member

wfurt commented Feb 11, 2023

Describe the bug

This is similar to #2975 and it seems to be new in 2.2

$ ldd artifacts/bin/linux/x64_Debug_openssl/libmsquic.so.2.1.7
	linux-vdso.so.1 (0x00007ffd7b1c4000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fc7fa425000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fc7fa402000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fc7fa210000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fc7fa7aa000)
$ ldd artifacts/bin/linux/x64_Debug_openssl/libmsquic.so.2.2.0
	linux-vdso.so.1 (0x00007ffcb51bf000)
	libcrypto.so.1.1 => /lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f0d22fd8000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f0d22fd2000)
	libnuma.so.1 => /lib/x86_64-linux-gnu/libnuma.so.1 (0x00007f0d22fc5000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f0d22fa2000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f0d22db0000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f0d23423000)

The consequence is that it

  1. can break uses on upgrade as new library needs more than the previous version
  2. allows to install msquic package without warning but leave it dysfunctional.

that is unpleasant and difficult for users to diagnose.

Affected OS

  • Windows
  • Linux
  • macOS
  • Other (specify below)

Additional OS information

No response

MsQuic version

main

Steps taken to reproduce bug

easiest is to run ldd to show library dependencies.

Expected behavior

packages properly state dependencies

Actual outcome

libnuma is missing

Additional details

We should either fix packages to pull in libnuma as needed or we should make it soft dependency e.g. optionally use it via function pointers.

cc: @ManickaP @CarnaViire

@nibanks nibanks added this to the Release 2.2 milestone Feb 11, 2023
@nibanks
Copy link
Member

nibanks commented Feb 11, 2023

Do we need to explicitly declare dependencies on every library you dumped above?

@wfurt
Copy link
Member Author

wfurt commented Feb 11, 2023

I don't think so. I would assume all the libc and linker are given. We can explore if other packages declare libpthread. I think it is probably part of libc as well.

@nibanks
Copy link
Member

nibanks commented Apr 17, 2023

I believe we got the code to not require libnuma, but numa support just lights up if it is installed. Can we make it a fixed dependency? Is it expected to be available on all platforms we want to support?

@wfurt
Copy link
Member Author

wfurt commented Apr 22, 2023

I don't understand what "fixed dependency" means. msquic can be configured in different ways. The published package dependencies should reflect the chosen set for given binary. We do not need to do that if the is light-up e.g. base functionality is there.

cc: @CarnaViire @ManickaP

@ManickaP
Copy link
Member

I think optional dependency might be a good solution for libnuma. WDYT?
And do we have test coverage for both options: with and without? Should we also test, at least once manually, all 4 combinations (build with/without, installed with/without) for expected outcomes?

@ManickaP
Copy link
Member

ManickaP commented Jun 2, 2023

This needs to get fixed before we release .NET 8. How come it surfaced in: dotnet/runtime#87038 ??? What I understood from @nibanks comment: #3421 (comment) is that the dependency is optional and the code knows how to work without libnuma present, regardless where/how it was build.

@ManickaP
Copy link
Member

ManickaP commented Jun 6, 2023

Is this fixed in all branches that have libnuma as a dependency?
Also, we shouldn't close this until we have fixed packages published in packages.microsoft.com.

Finally as a follow up, we need to revert all occurrences of libnuma dependency in our docker images, e.g.: dotnet/dotnet-buildtools-prereqs-docker#884

cc @CarnaViire @wfurt

@wfurt
Copy link
Member Author

wfurt commented Jun 8, 2023

another CI failure on arm64: dotnet/runtime#87275 (comment)

@wfurt
Copy link
Member Author

wfurt commented Jun 10, 2023

should be fixed by 2.2.2
Before

helixbot@33ece1741e82:/$ ldd /usr/lib/x86_64-linux-gnu/libmsquic.so.2
	linux-vdso.so.1 (0x00007fff4953f000)
	libcrypto.so.1.1 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f64987c6000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f64987c0000)
	libnuma.so.1 => not found
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f649879e000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f64985ca000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f6498c1f000)

and after upgrading msquic to 2.2.2 (no additional changes)

helixbot@33ece1741e82:/$ ldd /usr/lib/x86_64-linux-gnu/libmsquic.so.2.2.2
	linux-vdso.so.1 (0x00007fffdc393000)
	libcrypto.so.1.1 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007fe97a36d000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fe97a367000)
	libnuma.so.1 => /usr/lib/x86_64-linux-gnu/libnuma.so.1 (0x00007fe97a359000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fe97a337000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fe97a163000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fe97a7c7000)

@wfurt wfurt closed this as completed Jun 10, 2023
@nibanks nibanks moved this from Should be written to Done in MsQuic Walkthroughs Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants