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

Make test suite link to static LDNS rather than dynamic. #200

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

FGasper
Copy link
Contributor

@FGasper FGasper commented Dec 6, 2022

Issue #197: Cygwin fails these tests because it’s dicey to link dynamically to a library version that isn’t installed in the $PATH. Rather than adding platform-specific logic, this changeset alters the test suite to link statically rather than dynamically, which alleviates the problem.

Notably, this doesn’t resolve all problems with Cygwin in the test suite; other tests fail for other reasons. (See the GitHub issue for details.)

Issue NLnetLabs#197: Cygwin fails these tests because it’s dicey to link
dynamically to a library version that isn’t installed in the $PATH.
Rather than adding platform-specific logic, this changeset alters the
test suite to link statically rather than dynamically, which alleviates
the problem.

Notably, this doesn’t resolve all problems with Cygwin in the test suite;
other tests fail for other reasons. (See the GitHub issue for details.)
@pemensik
Copy link
Contributor

pemensik commented Jan 4, 2023

I do not think such change would work well for Linux distributions. We want the test as much similar as possible. I think some variable or configure parameter used only by the cygwin builds would be better solution. Actual loading of shared library is one of things we want to test during test suite run.

@FGasper
Copy link
Contributor Author

FGasper commented Jan 4, 2023

I do not think such change would work well for Linux distributions. We want the test as much similar as possible. I think some variable or configure parameter used only by the cygwin builds would be better solution. Actual loading of shared library is one of things we want to test during test suite run.

A fair point. I’ll see about adding that logic to the make scripts.

@wtoorop
Copy link
Member

wtoorop commented Jul 12, 2024

@FGasper have you looked at @pemensik 's comments yet?

@pemensik
Copy link
Contributor

Maybe extra LDNS_LDFLAGS and LDNS_LIBS variables should be defined in those makefiles. Still pointing to dynamic ldns by default. But if a need to override them would arise, just make LDNS_LIBS=$PWD/libldns.a would make it use correct version.

But it seems primary problem to solve is these tests do not use libtool, do not use propagated $(LIB) variable from main Makefile.in and therefore use just vaguely defined relative path ldns library via dynamic linking. If those tests would also use libtool, it could use whatever libtool produced into LIB variable. Instead of relying on LD_LIBRARY_PATH variable to point into correct place. Libtool is not optional, right?

Is there some reason why tpkg tests do not use it? Could it override LDNS_LIBS in .pre if LDNS_LIBS environment were set, for example?

If it pointed to libldns.la, it would use dynamic library configured via libtool. If that works in Cygwin, it would fix it also. Seems a better way, but significantly more work to fix it.

@pemensik
Copy link
Contributor

LDNS_LIBS ?= -lldns
LIBS = @LIBS@ @LIBSSL_SSL_LIBS@ $(LDNS_LIBS)

Should allow simple overriding via environment.

@wtoorop
Copy link
Member

wtoorop commented Jul 12, 2024

Is there some reason why tpkg tests do not use it? Could it override LDNS_LIBS in .pre if LDNS_LIBS environment were set, for example?

No, it just grew this way.

@FGasper
Copy link
Contributor Author

FGasper commented Jul 12, 2024

I no longer actively use LDNS. I also don't know the relevant build tools (eg libtool) well enough to comment.

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.

3 participants