| Message ID | 20210407101425.8988-1-andreas@fatal.se |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Drop NDEBUG debug prints which breaks fw_printenv | expand |
A short followup, Realized first now that you're using a shared mailinglist and I didn't mention anywhere that the patch is for github.com/sbabic/libubootenv ... Now it should be more obvious! Regards, Andreas Henriksson
Hi Andreas, On 07.04.21 12:14, Andreas Henriksson wrote: > Seems like cmake sometimes magically defines NDEBUG, but also cmake does not do this magically but how it is specified. Cmake foresees to have a Debug and Release build ruled by CMAKE_BUILD_TYPE, and if it is not specified, a Debug build is set and NDEBUG is not set. Just try this: mkdir Release cd Release cmake -DCMAKE_BUILD_TYPE=Release .. And you see in CmakeCache.txt: CMAKE_BUILD_TYPE:STRING=Release CMAKE_C_FLAGS_MINSIZEREL:STRING=-Os -DNDEBUG > it's way to easy to trigger a build where it's not defined and > thus the debug output is printed messing up fw_printenv output > and breaking just about every tool that tries to parse it's output. That means that you are building for Debug and not for Release. > Specially `fw_printenv -n foo` becomes useless (print only value). > Specially if the library is built with Debug on. If this is off, it works fine and it is full compatible with older tools. > FYI this is exactly what happened in https://bugs.debian.org/985948 Right, that the library is built without NDEBUG. I agree with Bastian's comment here. If I can say something, a tool reading the environment and using libubootenv should link to the library instead of running system(), but I could be OT here. Anyway, the solution is to set NDEBUG. I could also accept to drop these two lines, but if I need for some more specific reasons to add some further debug information, I cannot anymore due to breakage caused by packages built with wrong parameters. The fix is to set NDEBUG, not to drop the code. > > I quickly searched the mailinglist and it seems any attempt at adding > further NDEBUG's have been met with opposition as nothing sets NDEBUG. ???? Links, please. > Thus remove the last two remaining NDEBUG debug prints to avoid this > problem ever happening. NAK ...and avoid to add any debug information in future. Solution is described above. > > Signed-off-by: Andreas Henriksson <andreas@fatal.se> > --- > src/uboot_env.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/src/uboot_env.c b/src/uboot_env.c > index 30c39eb..8cb0887 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -1027,11 +1027,6 @@ static int libuboot_load(struct uboot_ctx *ctx) > } > } > > -#if !defined(NDEBUG) > - fprintf(stdout, "Environment %s, copy %d\n", > - ctx->valid ? "OK" : "WRONG", ctx->current); > -#endif > - > data = (uint8_t *)(buf[ctx->current] + offsetdata); > > char *flagsvar = NULL; > @@ -1069,9 +1064,6 @@ static int libuboot_load(struct uboot_ctx *ctx) > char *pvar; > char *pval; > if (flagsvar) { > -#if !defined(NDEBUG) > - fprintf(stdout, "Environment FLAGS %s\n", flagsvar); > -#endif > pvar = flagsvar; > > while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) { > Best regards, Stefano Babic
Hello Stefano Babic,
Thanks for your feedback which makes it clear where you stand on this!
On Wed, Apr 07, 2021 at 01:05:34PM +0200, Stefano Babic wrote:
[...]
> That means that you are building for Debug and not for Release.
[...]
FWIW the debhelper tool sets CMAKE_BUILD_TYPE to "none" (custom?) by
default.
I'm not sure why but I guess neither the Debug or Release variants are
suitable for a distribution that wants a Release build but with
debug symbols unstripped (so that we can automatically strip them and
put them in a separate dbgsym packages for all packages in the archive).
(I'll investigate some more and see if maybe debhelper could set
NDEBUG to avoid other packages falling in this trap.)
Regards,
Andreas Henriksson
PS. I agree more tools should use the library, but when they've used the
fw_printenv output "forever" then noone is in a hurry to do the porting
job needed. I'll happen eventually (and until then we'll have to
avoid breaking things)...
Hi Andreas, On 07.04.21 16:11, Andreas Henriksson wrote: > Hello Stefano Babic, > > Thanks for your feedback which makes it clear where you stand on this! > > On Wed, Apr 07, 2021 at 01:05:34PM +0200, Stefano Babic wrote: > [...] >> That means that you are building for Debug and not for Release. > [...] > > FWIW the debhelper tool sets CMAKE_BUILD_TYPE to "none" (custom?) by > default. Well, setting defines and build options is part of the debian package. CMAKE_BUILD_TYPE should be set or NDEBUG defined (first approach better). > I'm not sure why but I guess neither the Debug or Release variants are > suitable for a distribution that wants a Release build but with > debug symbols unstripped (so that we can automatically strip them and > put them in a separate dbgsym packages for all packages in the archive). And why are you saying they are already stripped ? Just do it: cmake -DCMAKE_BUILD_TYPE=Release .. make file src/libubootenv.so.0.3.1 src/libubootenv.so.0.3.1: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, BuildID[sha1]=d6ab62f64ed5a2b0778b747c778f9b3e7eefe505, not stripped file src/fw_setenv src/fw_setenv: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=1d682f6e83256177338fc5b3ae6ce36ce9922ff9, for GNU/Linux 3.2.0, not stripped Anyway, does Bastian's patch not fix the problem ? This sets NDEBUG just for the debian package, it looks fine to me . > > (I'll investigate some more and see if maybe debhelper could set > NDEBUG to avoid other packages falling in this trap.) > Ok > Regards, > Andreas Henriksson > > PS. I agree more tools should use the library, but when they've used the > fw_printenv output "forever" then noone is in a hurry to do the porting > job needed. I'll happen eventually (and until then we'll have to > avoid breaking things)... Of course - just the fix should be at the right place. Best regards, Stefano Babic
Hello again, On Wed, Apr 07, 2021 at 05:40:34PM +0200, Stefano Babic wrote: > Hi Andreas, [...] > Anyway, does Bastian's patch not fix the problem ? This sets NDEBUG > just for the debian package, it looks fine to me . No, there are 2 problems with it: - The _RELEASE variables are unused (because built type is not release as previously mentioned), so extending them is useless. (Renaming them to _NONE works, but then you hit next problem...) - It lacks -D [...] > Of course - just the fix should be at the right place. No worries. Already submitted a merge-request to fix the package before even posting to this list. Just wanted to investigate the different options to fix the problem. Quite off-topic for this list, but In case you're interested: https://salsa.debian.org/debian/libubootenv/-/merge_requests/3 (I'll take care of uploading the changes if the Debian package maintainer doesn't reply soon.) (Possibly debhelper cmake module could be improved to avoid every package having to fiddle with this if it's a generic cmake thing. That's for a rainy day to investigate further....) Thanks again for your quick feedback! Much appreciated. Regards, Andreas Henriksson
diff --git a/src/uboot_env.c b/src/uboot_env.c index 30c39eb..8cb0887 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -1027,11 +1027,6 @@ static int libuboot_load(struct uboot_ctx *ctx) } } -#if !defined(NDEBUG) - fprintf(stdout, "Environment %s, copy %d\n", - ctx->valid ? "OK" : "WRONG", ctx->current); -#endif - data = (uint8_t *)(buf[ctx->current] + offsetdata); char *flagsvar = NULL; @@ -1069,9 +1064,6 @@ static int libuboot_load(struct uboot_ctx *ctx) char *pvar; char *pval; if (flagsvar) { -#if !defined(NDEBUG) - fprintf(stdout, "Environment FLAGS %s\n", flagsvar); -#endif pvar = flagsvar; while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) {
Seems like cmake sometimes magically defines NDEBUG, but also it's way to easy to trigger a build where it's not defined and thus the debug output is printed messing up fw_printenv output and breaking just about every tool that tries to parse it's output. Specially `fw_printenv -n foo` becomes useless (print only value). FYI this is exactly what happened in https://bugs.debian.org/985948 I quickly searched the mailinglist and it seems any attempt at adding further NDEBUG's have been met with opposition as nothing sets NDEBUG. Thus remove the last two remaining NDEBUG debug prints to avoid this problem ever happening. Signed-off-by: Andreas Henriksson <andreas@fatal.se> --- src/uboot_env.c | 8 -------- 1 file changed, 8 deletions(-)