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

Makefile.am, configure.ac: fix hard coded static and include path #1026

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

vjardin
Copy link

@vjardin vjardin commented Nov 11, 2024

This serie is used to avoid hard coded path that do not work for any distributions. For instance, when buildroot is used or when cross compilation is used, the path of the headers cannot be enforce with /usr/include .

@vjardin
Copy link
Author

vjardin commented Nov 12, 2024

Side note : without this patch, it cannot link because
libmtcr_ul.a is not available, only libmtcr_ul.so (or its libmtcr_ul.la) had been compiled.

I do not understand why some -static are enforced while dynamic
linkage could be used.

We should not have any -static in order to be able to link
for the targets.
It is not compliant with cross compilation, it will lead to the
following:

aarch64-buildroot-linux-gnu-g++ -c -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -O2 -g0 -D_FORTIFY_SOURCE=1 -DHAVE_TERMIOS_H -DHAVE_SYS_PCI_H -isystem /usr/include/libxml2 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 conftest.cpp
aarch64-buildroot-linux-gnu-g++: ERROR: unsafe header/library path used in cross-compilation: '-isystem' '/usr/include/libxml2'

configure.ac: proper probe libxml2

libxml2: CFLAGS and CXXFLAGS are used
Makefiles are not using xml_CFLAGS
the include path cannot be hardcoded with:
  /usr/include/curl/
engine is exported by openssl/engine.h ; if not, we can have some
compilation warnings of some undefined symbols.
@vjardin vjardin changed the title Makefile.am: fix non static linkage Makefile.am, configure.ac: fix hard coded static and include path Nov 14, 2024
Thanks to this definition, we'll be able to ave cleaner Makefiles.
the .so file should remain for the target when dynamic linkage is used.
If the file is removed, none of the applications will be loadable.
start using DYNAMIC in order to be aligned with the
  --enable-all-static
argument.
DPDK assumes that the library shall be into /usr/lib instead of /usr/lib/mstflint/
@vjardin
Copy link
Author

vjardin commented Nov 16, 2024

Meanwhile, this pull request is included with the Buildroot package until it'll become mainstream.
https://patchwork.ozlabs.org/project/buildroot/list/?series=433071

@ogalbxela
Copy link
Contributor

ogalbxela commented Nov 19, 2024

Thank you for your contribution. Below is some background information to clarify the concerns you're addressing with your patches.

Mstflint is an open-source part of NVIDIA firmware tools (https://network.nvidia.com/products/adapter-software/firmware-tools/ - a proprietary code). Our typical process involves incorporating certain internal product features into the mstflint codebase with each release. While mstflint releases generally (but not always) align with mft releases, the mft undergoes a thorough testing cycle across various os-distribution-adapter combinations. For mstflint we only perform basic functionality testing.

We do not deploy dynamic libraries for either mft or mstflint as well as we do not test neither of products in that configuration, all tools are statically linked with all its components. If you are aiming to link your code with the mtcr_ul library, please use its static version. Unfortunately, we cannot release something that has not been tested. Changing our internal process with this regard may take a little bit less than forever.

Regarding your concern about the path used for placing the library: both mft and mstflint deploy its own versions of mtcr_ul library. To distinguish between them when both products are installed, we place the open-source version under /usr/lib/mstflint.

To summarize: we can only consider the portion of your patch related to the hardcoded paths. Additionally, we do not work directly on the master branch. All development is done on the master_devel. For each patch from the community, we first absorb it to the internal product, subject it to our internal test cycle, and then it becomes a part of official release. This usually happens (though not always) once every three months.

@vjardin
Copy link
Author

vjardin commented Nov 25, 2024

Thanks @ogalbxela for the feedbacks.

The goal is to properly support DPDK with Buildroot, see this series:
https://patchwork.ozlabs.org/project/buildroot/list/?series=433904

so, once mstflint, libmtcr_ul.so is available, DPDK's meson can probe it thanks to:
https://patchwork.ozlabs.org/project/buildroot/patch/[email protected]/
needed by:
https://git.dpdk.org/dpdk/tree/drivers/common/mlx5/linux/meson.build#n48

Do you want me to rebase this patch on the branch master_devel ?

Regarding dynamic vs static: what's about having a Makefile variable ? By default, it would be static, so it does not break your release and test processes ; but then, on special cases we can enable a build without static ?

@vjardin
Copy link
Author

vjardin commented Nov 27, 2024

Please @ogalbxela , do you have some feedbacks ?

@ogalbxela
Copy link
Contributor

ogalbxela commented Nov 27, 2024

I'll bring this up with our management team and let you know.
In the meantime, could you clarify what is preventing you from statically linking mtcr_ul?
Good reasoning might help to get it go through.

@vjardin
Copy link
Author

vjardin commented Dec 3, 2024

what is preventing you from statically linking mtcr_ul?

issues with the current static linking:

  • it did not work on Buildroot with DPDK that is looking for the .so; so I'd prefer to fix mstflint configure/makefiles which have this issue any many other ones
  • it adds a memory footprint overhead because I do run the system on Ramdisk in order to avoid any issues with the physical disks

And frankly, why would you support static linking only nowadays ?

We need the solution to be more flexible to be compiled into some embedded environments. For my case, it is Buildroot, but I guess that some others would have some similar issues for some other embedded Linux.

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

Successfully merging this pull request may close these issues.

2 participants