nmk uses LD directly, ignores -Wl LDFLAGS arguments

[paravoid]

Description

nmk in using LD to link, rather than CC. This causes at least two different issues that I can see:

1. -Wl,XXX,YYY arguments do not work with LD directly so there is a $(filter-out ...) in build.mk to filter those out. This is counter-intuitive, since it's ignoring LDFLAGS explicitly passed by the user through the environment. A real-world example is hardening flags like -Wl,-z,relro and -Wl,-z,bindnow which are increasingly the default (such in Debian through dpkg-buildflags).
2. LTO builds (-flto and friends) fail, as gcc really needs to be the one "linking" there.

The decision seems rather explicit and I don't know enough about neither criu nor the build system, so I hesitate submitting a patch to s/LD/CC/. I tried that locally and it seemed to work.

Steps to reproduce the issue:

1. export LDFLAGS="-Wl,-z,relro"
2. Build
3. Observe that the build was not linked with -z relro

[carnil]

For reference: Downstream needed in Debian as https://salsa.debian.org/debian/criu/-/merge_requests/3/diffs?commit_id=b854c3ec29988cfc6f925237e74c336f6270d304 and https://salsa.debian.org/debian/criu/-/merge_requests/3/diffs?commit_id=dc3c60c69dc9fbadd8b1b34f1e15346314c311e4

[adrianreber]

I would be very careful with changes to the build system. CRIU is very special in a few area in the build system.

I am maintaining the Fedora and RHEL CRIU packages and many common distribution hardening features cannot blindly be enabled for all parts of CRIU.

About this specific change. I don't know. If the the complete test suite still works on all distributions then it should be okay.

I was reading some of the comments in that gitlab pull request and there are some ideas in there which are completely wrong. I would be very careful with build system changes.

The Fedora package is based on https://src.fedoraproject.org/rpms/criu/blob/rawhide/f/criu.spec and works very well with crun. Same for the RHEL package https://gitlab.com/redhat/centos-stream/rpms/criu/-/blob/c9s/criu.spec

[carnil]

@adrianreber thanks for the comments + warnings, those are helpful!

I would be very careful with changes to the build system. CRIU is very special in a few area in the build system.

I am maintaining the Fedora and RHEL CRIU packages and many common distribution hardening features cannot blindly be enabled for all parts of CRIU.

About this specific change. I don't know. If the the complete test suite still works on all distributions then it should be okay.

I learn from that as downstream packager, that we probably should only do it if upstream acknowledges it and is being taken.

I was reading some of the comments in that gitlab pull request and there are some ideas in there which are completely wrong. I would be very careful with build system changes.

Would you mind highlighting which of the ideas are completely wrong there?

The Fedora package is based on https://src.fedoraproject.org/rpms/criu/blob/rawhide/f/criu.spec and works very well with crun. Same for the RHEL package https://gitlab.com/redhat/centos-stream/rpms/criu/-/blob/c9s/criu.spec

For Debian I guess we can take some of the hints there to implement our changes. It is longstanding in https://bugs.debian.org/782007 to do a proper split up of the packaging so other (like crun can properly use it)

[adrianreber]

I was reading some of the comments in that gitlab pull request and there are some ideas in there which are completely wrong. I would be very careful with build system changes.

Would you mind highlighting which of the ideas are completely wrong there?

From https://salsa.debian.org/debian/criu/-/merge_requests/3

Split the compel binary into a different package (is it useful on its own? no idea)

This does not make sense. This is only used internally during build.

Move crit and/or criu-ns to python3-pycriu (would make them too hard to find) or a separate package, to avoid the python dependency from the main criu package.

Not sure about this. This does not seem like a problem. In Fedora there is a python3-criu package and a crit package. This sounds similar to this change.

Patch the upstream sources to link compel against libcompel

This is again only used internally.

and criu against libcriu (right now they link statically instead)

This does not really make sense. Not sure if I understand it correctly, but it is not possible to link criu against libcriu.so. libcriu is just a wrapper which calls the binary. You cannot link criu against libcriu.

Looking at the Fedora/RHEL package we provide a configuration for tmpfiles (https://gitlab.com/redhat/centos-stream/rpms/criu/-/blob/c9s/criu-tmpfiles.conf) and we also fix the pkgconfig file (https://src.fedoraproject.org/rpms/criu/blob/rawhide/f/criu.pc.patch). This patch is not applied upstream as we thought there is a difference in library naming between distributions. I think that change was related to crun.

For crun you basically need (maybe separate package) libcriu.so. libcriu.so needs the criu binary. In the Fedora crun spec file criu-devel is needed during build time, but libcriu.so is not a hard dependency but a Recommends:. The libcriu and criu package can be uninstalled but crun still works. crun uses dlopen() to load libcriu.so if it is there. The header files need to exist during buildtime to enable the feature at all.

[github-actions[bot]]

A friendly reminder that this issue had no activity for 30 days.