PR#50335: Fix build on alpine linux - 389-ds-base

389-ds-base

#50335 Fix build on alpine linux

Closed 3 years ago by spichugi. Opened 5 years ago by codehotter.

codehotter/389-ds-base fix-alpine-build into master

Emanuel Rietveld • 5 years ago

c563996

include/ldaputil/certmap.h

file modified

+1 -1

		`@@ -16,7 +16,7 @@`
		`/* What was extcmap.h begins ... */`

		`#include <ldap.h>`
		`- #include <nss3/cert.h>`
		`+ #include <cert.h>`

		`#ifndef NSAPI_PUBLIC`
		`#define NSAPI_PUBLIC`

ldap/servers/slapd/libglobs.c

file modified

+5 -5

		`@@ -128,7 +128,7 @@`
		`#endif /* USE_SYSCONF */`
		`#include "slap.h"`
		`#include "plhash.h"`
		`- #if defined(LINUX)`
		`+ #if defined(__GLIBC__)`
		`#include <malloc.h>`
		`#endif`
		`#include <sys/resource.h>`
		`@@ -242,7 +242,7 @@`
		`slapi_onoff_t init_cn_uses_dn_syntax_in_dns;`
		`slapi_onoff_t init_global_backend_local;`
		`slapi_onoff_t init_enable_nunc_stans;`
		`- #if defined(LINUX)`
		`+ #if defined(__GLIBC__)`
		`slapi_int_t init_malloc_mxfast;`
		`slapi_int_t init_malloc_trim_threshold;`
		`slapi_int_t init_malloc_mmap_threshold;`
		`@@ -1115,7 +1115,7 @@`
		`NULL, 0,`
		`(void **)&global_slapdFrontendConfig.cn_uses_dn_syntax_in_dns, CONFIG_ON_OFF,`
		`(ConfigGetFunc)config_get_cn_uses_dn_syntax_in_dns, &init_cn_uses_dn_syntax_in_dns},`
		`- #if defined(LINUX)`
		`+ #if defined(__GLIBC__)`
		`{CONFIG_MALLOC_MXFAST, config_set_malloc_mxfast,`
		`NULL, 0,`
		`(void **)&global_slapdFrontendConfig.malloc_mxfast,`
		`@@ -1744,7 +1744,7 @@`
		`cfg->logging_backend = slapi_ch_strdup(SLAPD_INIT_LOGGING_BACKEND_INTERNAL);`
		`cfg->rootdn = slapi_ch_strdup(SLAPD_DEFAULT_DIRECTORY_MANAGER);`
		`init_enable_nunc_stans = cfg->enable_nunc_stans = LDAP_OFF;`
		`- #if defined(LINUX)`
		`+ #if defined(__GLIBC__)`
		`init_malloc_mxfast = cfg->malloc_mxfast = DEFAULT_MALLOC_UNSET;`
		`init_malloc_trim_threshold = cfg->malloc_trim_threshold = DEFAULT_MALLOC_UNSET;`
		`init_malloc_mmap_threshold = cfg->malloc_mmap_threshold = DEFAULT_MALLOC_UNSET;`
		`@@ -8088,7 +8088,7 @@`
		`return retVal;`
		`}`

		`- #if defined(LINUX)`
		`+ #if defined(__GLIBC__)`
		`int`
		`config_set_malloc_mxfast(const char attrname, char value, char *errorbuf, int apply __attribute__((unused)))`
		`{`

ldap/servers/slapd/main.c

file modified

+2 -2

		`@@ -60,7 +60,7 @@`
		`#include "fe.h"`
		`#include <nss.h>`

		`- #ifdef LINUX`
		`+ #ifdef __GLIBC__`
		`/* For mallopt. Should be removed soon. */`
		`#include <malloc.h>`
		`#endif`
		`@@ -668,7 +668,7 @@`
		`daemon_ports_t ports_info = {0};`
		`ns_thrpool_t *tp = NULL;`

		`- #ifdef LINUX`
		`+ #ifdef __GLIBC__`
		`char *m = getenv("SLAPD_MXFAST");`
		`if (m) {`
		`int val = atoi(m);`

codehotter commented 5 years ago

include nss as <cert.h> instead of <nss3/cert.h> - pkgconfig will pick up the proper include path
change #ifdef LINUX into #ifdef __GLIBC__ for malloc options so that we can build on musl

codehotter commented 4 years ago

Perhaps this should be done with configure instead of GLIBC define.

firstyear commented 4 years ago

@codehotter You might find that configure is adding nss3 into the include path, which is why the sources use cert.h not nss3/cert.h.

I'm also not sure about the ifdef linux to glibc change, because of the (potential) to build on freebsd which requires a distinction for those platform features. It could be better to determine why on alpine linux is not defined as true?

codehotter commented 4 years ago

On alpine linux is defined as true, however, alpine uses musl which does not support mallopt. Since freebsd uses glibc but also does not support mallopt, that just further confirms that the glibc define is not the proper solution.

Perhaps ./configure could check for mallopt and it should be behind a mallopt ifdef instead. I'll try to see if I can get that to work. I will also test if it then builds on alpine, freebsd and fedora.

I'm not sure if I understand your nss3 comment. Do you support the change to include cert.h instead of nss3/cert.h, since configure is already adding nss3 to the include path? On alpine the include path is /usr/include/nss, which is why it breaks if we hardcode nss3 in the source.

firstyear commented 4 years ago

@codehotter Okay, so how about we make that "if linux && glibc" then around mallopt?

@mhonek What do you think about the nss3/cert.h change here?

(I'll defer to people to who know more about this than me :) )

hmc commented 4 years ago

The NSS path issue also occurs on Debian-based systems, since cert.h exists in /use/include/nss. Note that this needs to be fixed in two places in the codebase.

@firstyear Where is cert.h for you?

firstyear commented 4 years ago

/usr/include/nss3/cert.h is the location on SUSE.

If it's an issue on debian that it's in /usr/include/nss then I think changing this to "nss3/cert.h" would break debian, because we need to -I the /usr/include/nss[3] into our configure process, so I think that changing this to <cert.h> and fixing our -I path is the right option here. I'd still like @mhonek's feedback though.

hmc commented 4 years ago

@firstyear Changing to <cert.h> (with no preceding path) is the correct fix. The recent pkg-config changes automatically generate the correct include path for nss.

I think this change should be a separate pull request.

mhonek commented 4 years ago

For the record, the <cert.h> issue was fixed in Hugh's PR #50352, commit 06c9f53 in master.

I'm not sure about the LINUX vs. __GLIBC__. @codehotter what do you think about the William's proposal above?:

@codehotter Okay, so how about we make that "if linux && glibc" then around mallopt?

codehotter commented 4 years ago

The proper fix for this might be to check for it directly with configure, but that simple fix would certainly solve the problem for me.

firstyear commented 4 years ago

So maybe we should rebase this ontop of #50352 (which is now master) and re-asses if more changes are needed around cert.h? Is that okay?

firstyear commented 4 years ago

#50352 has been merged, did you want me to finish this PR up?

firstyear commented 4 years ago

@codehotter Is this still an issue?

codehotter commented 4 years ago

I've become much less enamored with alpine in general in the meantime, so I'm happy to close this, although I suspect some configure fix or ifdefs would still be necessary to make this work on alpine.

firstyear commented 4 years ago

Sure. I'll close it and we can revive if there are requests. Currently I think the containerisation efforts are mainly driven by myself which means they'll be suse based, and that seems to be working today.