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

- Reduce unnecessary linking #511

Merged
merged 1 commit into from Dec 1, 2021
Merged

- Reduce unnecessary linking #511

merged 1 commit into from Dec 1, 2021

Conversation

yan12125
Copy link
Contributor

When pyunbound is enabled while pythonmodule is not (i.e., ./configure --without-pythonmodule --with-pyunbound), only the Python library _unbound.so uses Python functions, and main programs (unbound, unbound-anchor, ...) and libunbound.so do not. This patch removes unneeded linking.

Note that I didn't commit re-generated autoconf files as that brings too many unrelated changes to ./configure. If you wish, I will push updated autoconf files.

@yan12125 yan12125 mentioned this pull request Jul 23, 2021
@ArchangeGabriel
Copy link
Contributor

ArchangeGabriel commented Jul 24, 2021

At the same time, there is overlinking happening there:

python-unbound W: Unused shared library '/usr/lib/libssl.so.1.1' by file ('usr/lib/python3.9/site-packages/_unbound.so')
python-unbound W: Unused shared library '/usr/lib/libsodium.so.23' by file ('usr/lib/python3.9/site-packages/_unbound.so')
python-unbound W: Unused shared library '/usr/lib/libprotobuf-c.so.1' by file ('usr/lib/python3.9/site-packages/_unbound.so')
python-unbound W: Unused shared library '/usr/lib/libevent-2.1.so.7' by file ('usr/lib/python3.9/site-packages/_unbound.so')
python-unbound W: Unused shared library '/usr/lib/libsystemd.so.0' by file ('usr/lib/python3.9/site-packages/_unbound.so')
python-unbound W: Unused shared library '/usr/lib/libcrypto.so.1.1' by file ('usr/lib/python3.9/site-packages/_unbound.so')
python-unbound W: Unused shared library '/usr/lib/libhiredis.so.1.0.0' by file ('usr/lib/python3.9/site-packages/_unbound.so')
python-unbound W: Unused shared library '/usr/lib/libnghttp2.so.14' by file ('usr/lib/python3.9/site-packages/_unbound.so')

EDIT: The usual sed -i -e 's/ -shared / -Wl,-O1,--as-needed\0/g' libtool workaround works for this though.

@yan12125 yan12125 changed the title - Link to libpython only when needed, fixes #242 - Reduce unnecessary linking Jul 28, 2021
@yan12125
Copy link
Contributor Author

At the same time, there is overlinking happening there:

Many thanks for testing! I looked into it, and found two issues:

  • Unbound's Makefile specifies unneeded libraries for _unbound.so. I address that issue in the updated patch.
  • libtool brings unneeded libraries. For example, using ./configure --with-pythonmodule --with-pyunbound, build commands for _unbound.so are:
./libtool --tag=CC --mode=link gcc  -I. -I/usr/include/python3.9 -DSRCDIR=. -g -O2 -flto -pthread  -module -avoid-version -no-undefined -shared -o _unbound.la libunbound_wrap.lo -rpath /usr/lib/python3.9/site-packages libunbound.la -L/usr/lib -L/usr/lib/python3.9 -L. -lpython3.9
libtool: link: rm -fr  .libs/_unbound.la .libs/_unbound.lai .libs/_unbound.so
libtool: link: gcc -shared  -fPIC -DPIC  .libs/libunbound_wrap.o   -Wl,-rpath -Wl,/home/yen/var/unbound/.libs -Wl,-rpath -Wl,/usr/local/lib ./.libs/libunbound.so -L/usr/lib -L/usr/lib/python3.9 -L. -lssl -lcrypto -lpython3.9  -g -O2 -flto -pthread   -pthread -Wl,-soname -Wl,_unbound.so -o .libs/_unbound.so

Here -lssl and -lcrypto do not appear in the first libtool command, but the final gcc command includes them. Googling around, besides the sed command you've mentioned, I also found discussions about linking "recursively" by libtool and a patch from Debian to disable it. I guess that is the cause why -lssl and -lcrypto are added to _unbound.so - both libraries are used by libunbound. Hopefully, with this patch, linking is correct for distros that disable recursive linking without further patching.

/cc @orbea Mind to test this patch on Gentoo with slibtool? I found that $(LIBS) is added for _unbound.so in #449. I guess replacing $(LIBS) with $(PYTHON_LIBS) will not break things on Gentoo, but more testing will be better.

@orbea
Copy link
Contributor

orbea commented Jul 29, 2021

This patch seems to compile correctly using slibtool, the current git head (dd25413) and --with-pyunbound.

I think the only way this could fail with slibtool is if $(LIBS) contains any required references that $(PYTHON_LIBS) does not also contain which would then fail with -no-undefined which GNU libtool silently ignores, but that doesn't seem to be the case.

@yan12125
Copy link
Contributor Author

Thanks a lot for testing and clear explanations!

- Link to libpython only when needed, fixes #242

When pyunbound is enabled while pythonmodule is not (i.e., ./configure
--without-pythonmodule --with-pyunbound), only the Python library
_unbound.so uses Python functions, and main programs (unbound,
unbound-anchor, ...) and libunbound.so do not. This patch removes
unneeded linking.

- Link the Python library _unbound.so to Python only. _unbound.so does
  not directly use libraries used by libunbound. This patch removes
  unneeded linking mentioned in [1]

[1] #511 (comment)
@yan12125
Copy link
Contributor Author

yan12125 commented Aug 6, 2021

Rebased to resolve conflicts after 8e538dc

@gthess gthess merged commit 94ea0e5 into NLnetLabs:master Dec 1, 2021
@gthess
Copy link
Member

gthess commented Dec 1, 2021

Thanks for this! LGTM and the Python tests pass.

gthess added a commit that referenced this pull request Dec 1, 2021
    - Merge PR #511 from yan12125: Reduce unnecessary linking.
@yan12125 yan12125 deleted the python-linking branch December 1, 2021 05:31
jedisct1 added a commit to jedisct1/unbound that referenced this pull request Jan 13, 2022
* nlnet/master: (36 commits)
  - Add missing configure flags for optional features in the   documentation. - Fix Unbound capitalization in the documentation.
  - Fix to pick up other class local zone information before unlock.
  - Allow local-data for classes other than IN to inherit a configured   local-zone's type if possible, instead of defaulting to type   transparent as per the implicit rule.
  - Add code similar to fix for ldns for tab between strings, for   consistency, the test case was not broken.
  Continue with version 1.14.1
  - Fix validator debug output about DS support, print correct algorithm.
  Changelog note for NLnetLabs#581
  Fix -Wshadow
  Fix -Wmissing-prototypes by declaring functions static.
  - Fix compile warning for if_nametoindex on windows 64bit.
  - Fix doc/unbound.doxygen to remove obsolete tag warning.
  - configure is set to 1.14.0, and release branch.
  - Fix NLnetLabs#574: Review fixes for size allocation.
  - Fix NLnetLabs#454: listen_dnsport.c:825: error: ‘IPV6_TCLASS’ undeclared.
  Changelog note for NLnetLabs#530:     - Merge PR NLnetLabs#530 from Shchelk: Fix: dereferencing a null pointer.
  Changelog note for NLnetLabs#522:     - Merge PR NLnetLabs#522 from sibeream: memory management violations fixed.
  Changelog note for NLnetLabs#562:     - Merge PR NLnetLabs#562 from Willem: Reset keepalive per new tcp session.
  Changelog note for NLnetLabs#555:     - Merge PR NLnetLabs#555 from fobser: Allow interface names as scope-id in       IPv6 link-local addresses.
  Changelog note for NLnetLabs#493:     - Merge PR NLnetLabs#493 from Jaap: Fix generation of libunbound.pc.
  Changelog note for NLnetLabs#511:     - Merge PR NLnetLabs#511 from yan12125: Reduce unnecessary linking.
  ...
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.

None yet

4 participants