share/mk/bsd.links.mk: add RSYMLINKS support

[ngie]

Summary

share/mk/bsd.links.mk: add RSYMLINKS support

RSYMLINKS is a user-provided knob for installing relative symlinks via
INSTALL_RSYMLINK. It's introduction is being precipitated by changes proposed
by rgrimes in making absolute symlinks non-absolute again, e.g., r314833.

MFC after: 1 month
Sponsored by: Dell EMC Isilon

Note: Documentation in bsd.README will be available soon (pending review/commit of D9918).

Test Plan

$ sudo make install
install  -s -o root -g wheel -m 444     libenc_test.so /usr/tests/lib/libxo/
install  -o root -g wheel -m 444    libenc_test.so.debug /usr/lib/debug/usr/tests/lib/libxo/
/usr/lib/libxo/encoder/test.enc -> /usr/tests/lib/libxo/libenc_test.so
$ ls -l /usr/lib/libxo/encoder/test.enc
lrwxr-xr-x  1 root  wheel  39 Mar  7 00:51 /usr/lib/libxo/encoder/test.enc -> ../../../tests/lib/libxo/libenc_test.so
$ realpath /usr/lib/libxo/encoder/test.enc
/usr/tests/lib/libxo/libenc_test.so
$ file `realpath /usr/lib/libxo/encoder/test.enc`
/usr/tests/lib/libxo/libenc_test.so: ELF 64-bit LSB shared object, x86-64, version 1 (FreeBSD), dynamically linked, stripped
$ sudo make install TESTSBASE=/foo
install  -s -o root -g wheel -m 444     libenc_test.so /foo/lib/libxo/
install  -o root -g wheel -m 444    libenc_test.so.debug /foo/lib/libxo/.debug/
/usr/lib/libxo/encoder/test.enc -> /foo/lib/libxo/libenc_test.so
$ ls -l /usr/lib/libxo/encoder/test.enc
lrwxr-xr-x  1 root  wheel  40 Mar  7 00:52 /usr/lib/libxo/encoder/test.enc -> ../../../../foo/lib/libxo/libenc_test.so
$ realpath /usr/lib/libxo/encoder/test.enc
/foo/lib/libxo/libenc_test.so
$ file `realpath /usr/lib/libxo/encoder/test.enc`
/foo/lib/libxo/libenc_test.so: ELF 64-bit LSB shared object, x86-64, version 1 (FreeBSD), dynamically linked, stripped

[ngie]

share/mk/bsd.links.mk

27

For the record, I'm not entirely sure why this is not prefixed with ${DESTDIR}, except to maybe deal with the case that rgrimes is trying to fix to restore previous behavior after the release-pkg effort.

[rgrimes]

share/mk/bsd.links.mk

23

You do not want DESTDIR on the ${s} only on the ${t} otherwise you end up with DESTDIR in the link.
But I dont think we need RSYMLINKS at all.

27

The ${s} is not prefixed with DESTDIR to keep DESTDIR from appearing in the body of the link.

You can kill the RSYMLINKS by simply changing INSTALL_SYMLINKS to INSTALL_RSYMLINKS
and it should fix this once and for all. Then I can go rip out all the ../.. in SYMLINKS targets.

[bdrewery]

Yes this is trivial on the surface but it requires work to support DIRDEPS_BUILD as well. While that feature is opt-in and not widely used, the need for RSYMLINKS doesn't seem to be high enough to warrant breaking DIRDEPS_BUILD. Please add sjg@ onto the review. He likely can quickly code up the needed code for DIRDEPS_BUILD.

[rgrimes]

The following trivial patch fixes the relativeness of SYMLINKS and given this is already a supported META target no other changes are need.
This has been regression tested with make installworld DESTDIR={before,after} and the trees are the same with special attention taken to
run a find DESTDIR -type l -ls .... and diffing the results. No symbolic link information was altered by this change.

Index: share/mk/bsd.links.mk

• share/mk/bsd.links.mk (revision 314718)

+++ share/mk/bsd.links.mk (working copy)
@@ -20,5 +20,5 @@
.endfor
.for s t in ${SYMLINKS}

@${ECHO} "${t} -> ${s}" ;\

• ${INSTALL_SYMLINK} ${TAG_ARGS} ${s} ${DESTDIR}/${t}

+ ${INSTALL_RSYMLINK} ${TAG_ARGS} ${s} ${DESTDIR}/${t}
.endfor

[ngie]

share/mk/bsd.links.mk

23

No, you need DESTDIR, because that's how install(1) determines the common path, then develops the relative path off of that.

Try running install -l rs /bin/test /foo/bin/[ -- you'll discover that the relative path has an extra ../ in it to account for DESTDIR=/foo.

27

Yeah, this was a part of my braino this morning.

No, I'm not just going to replace INSTALL_SYMLINKS with INSTALL_RSYMLINKS. As I said earlier on src-committers@, doing so breaks an interface that has been in place for 4+ years:

SYMLINKS has been around since r245752. I personally don’t know if it’s wise to remove functionality that’s been “in production” for 4+ years.

Making the target intelligently use one command over the other might seem ok, but it could have unintended consequences. I think it’s best to have another well-documented variable that uses INSTALL_RSYMLINK vs INSTALL_SYMLINK.

Also, in the CR, please note that the SYMLINKS piece doesn’t prefix source targets with ${DESTDIR}, whereas the LINKS (and soon to be RSYMLINKS portion) will need it in order to compute the right paths and execute the right behavior. Otherwise, you’re going to be breaking someone’s use for SYMLINKS for no good reason.

[ngie]

In D9919#205033, @rgrimes wrote:

The following trivial patch fixes the relativeness of SYMLINKS and given this is already a supported META target no other changes are need.
This has been regression tested with make installworld DESTDIR={before,after} and the trees are the same with special attention taken to
run a find DESTDIR -type l -ls .... and diffing the results. No symbolic link information was altered by this change.

More testing is required for the change -- in particular, you might be breaking ports that require absolute paths to symlinks.

You should request an "-exp run" to determine what breakage might be present after the change.

[ngie]

In D9919#205008, @bdrewery wrote:

Yes this is trivial on the surface but it requires work to support DIRDEPS_BUILD as well. While that feature is opt-in and not widely used, the need for RSYMLINKS doesn't seem to be high enough to warrant breaking DIRDEPS_BUILD. Please add sjg@ onto the review. He likely can quickly code up the needed code for DIRDEPS_BUILD.

Will do -- thanks for the heads up!

[ngie]

Abandoning revision since I will not be working on the build system proper anymore, apart from test integration or any components that directly correlate with testing.