build failure without explicit -DBC_ENABLE_LIBRARY=0

[enh-google]

this might be WAI as an unsupported configuration, but i accidentally ran into this because i have to maintain a parallel build for Android's build system...

with Android's existing Android.bp file:

FAILED: out/soong/.intermediates/external/bc/bc/android_x86_64_silvermont/obj/external/bc/src/data.o
PWD=/proc/self/cwd prebuilts/clang/host/linux-x86/clang-r428724/bin/clang -c  -Werror=implicit-function-declaration -DANDROID -fmessage-length=0 -W -Wall -Wno-unused -Winit-self -Wpointer-arith -Wunreachable-code-loop-increment -no-canonical-prefixes -DNDEBUG -UDEBUG -fno-exceptions -Wno-multichar -O2 -g -fdebug-info-for-profiling -fno-strict-aliasing -Werror=date-time -Werror=pragma-pack -Werror=pragma-pack-suspicious-include -Werror=string-plus-int -Werror=unreachable-code-loop-increment -D__compiler_offsetof=__builtin_offsetof -faddrsig -fcommon -Werror=int-conversion -fexperimental-new-pass-manager -Wno-reserved-id-macro -Wno-unused-command-line-argument -fcolor-diagnostics -Wno-sign-compare -Wno-defaulted-function-deleted -Wno-inconsistent-missing-override -Wno-c99-designator -Wno-gnu-folding-constant -Wunguarded-availability -D__ANDROID_UNAVAILABLE_SYMBOLS_ARE_WEAK__ -fdebug-prefix-map=/proc/self/cwd= -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ffunction-sections -fdata-sections -fno-short-enums -funwind-tables -fstack-protector-strong -Wa,--noexecstack -D_FORTIFY_SOURCE=2 -Wstrict-aliasing=2 -Werror=return-type -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -Werror=format-security -nostdlibinc -Wno-enum-compare -Wno-enum-compare-switch -Wno-null-pointer-arithmetic -Wno-null-dereference -Wno-pointer-compare -Wno-xor-used-as-pow -Wno-final-dtor-non-final-class -Wno-psabi -m64 -march=slm -mssse3 -msse4 -msse4.1 -msse4.2 -maes -mpopcnt -target x86_64-linux-android10000 -Bprebuilts/gcc/linux-x86/x86/x86_64-linux-android-4.9/x86_64-linux-android/bin -fPIE  -Iexternal/bc/include -Iexternal/bc -D__LIBC_API__=10000 -D__LIBM_API__=10000 -D__LIBDL_API__=10000 -Ibionic/libc/async_safe/include -Isystem/logging/liblog/include -Ibionic/libc/system_properties/include -Isystem/core/property_service/libpropertyinfoparser/include -Iout/soong/.intermediates/external/bc/bc-version.h/gen -isystem bionic/libc/include -isystem bionic/libc/kernel/uapi/asm-x86 -isystem bionic/libc/kernel/uapi -isystem bionic/libc/kernel/android/scsi -isystem bionic/libc/kernel/android/uapi -Wall -Werror -include bc-version.h -DBC_ENABLED=1 -DDC_ENABLED=0 -DBC_ENABLE_EXTRA_MATH=0 -DBC_ENABLE_HISTORY=0 -DBC_ENABLE_LONG_OPTIONS=1 -DBC_ENABLE_NLS=0 -DBC_ENABLE_SIGNALS=1 -Os -std=gnu99 -Isystem/core/include -Isystem/logging/liblog/include -Isystem/media/audio/include -Ihardware/libhardware/include -Ihardware/libhardware_legacy/include -Ihardware/ril/include -Iframeworks/native/include -Iframeworks/native/opengl/include -Iframeworks/av/include  -Werror=bool-operation -Werror=implicit-int-float-conversion -Werror=int-in-bool-context -Werror=int
In file included from external/bc/src/data.c:39:
In file included from external/bc/include/args.h:41:
external/bc/include/vm.h:401:2: error: unknown type name 'BcRNG'
        BcRNG rng;
        ^
1 error generated.

i needed to manually fix this:

diff --git a/Android.bp b/Android.bp
index c0a66126..7160fee1 100644
--- a/Android.bp
+++ b/Android.bp
@@ -24,6 +24,7 @@ cc_defaults {
     "-DDC_ENABLED=0",
     "-DBC_ENABLE_EXTRA_MATH=0",
     "-DBC_ENABLE_HISTORY=0",
+    "-DBC_ENABLE_LIBRARY=0",
     "-DBC_ENABLE_LONG_OPTIONS=1",
     "-DBC_ENABLE_NLS=0",
     "-DBC_ENABLE_SIGNALS=1",
@@ -88,7 +89,7 @@ sh_test {
   host_supported: true,
   device_supported: false,
   data: [
-    "functions.sh",
+    "scripts/functions.sh",
     "tests/**/*",
   ],
 }

the default Makefile hides this because of the CPPFLAGS8 line that ensures BC_ENABLE_LIBRARY is always set.

like i said, maybe WAI and fixed by making my build a bit more like yours, but i thought i'd mention it anyway in case you considered this an accident...

[enh-google]

(you can't really tell from the context in the diff, but the Android build file doesn't actually -D all of the things --- i think it's just "what was necessary when we first imported" + "this one now". so if you do want everything defined all the time and want to protect against user error, you might want to add some #ifndef ... #error verbiage somewhere, so me and anyone else with their own build system knows when you've added something, and knows they need to make a conscious decision?)

[gavinhoward]

This is a hard one.

So it is really WAI, even though I don't like it. The reason is because the user-visible header for the library, include/bcl.h, needs to define some things that are not used in the library, but if the library is not enabled, the library-only stuff needs to be skipped. So there are #if BC_ENABLE_LIBRARY/#endif guards in include/bcl.h.

However, that causes problems because if the user is building a project using bcl, they are not going to be defining BC_ENABLE_LIBRARY, so right before the #if BC_ENABLE_LIBRARY/#endif guards in include/bcl.h, there is this:

#ifndef BC_ENABLE_LIBRARY
#define BC_ENABLE_LIBRARY (1)
#endif // BC_ENABLE_LIBRARY

This is to ensure that users who build projects with bcl as a dependency don't have to do anything special with their builds.

Unfortunately, both of those things together caused the problem you see.

However, I think I found a solution: 7e218161a9. Since include/status.h includes include/bcl.h after that point, I can use it to disable building the library by default when BC_ENABLE_LIBRARY is not defined.

Can you test that 7e218161a9 fixes your problem? If so, I will make another release, though I may wait on any problems FreeBSD has with updating their in-tree version.

[enh-google]

I may wait on any problems FreeBSD has with updating their in-tree version.

just to confirm: you don't know of any problems they've had, you're just waiting to see if they have any? i ask because i'm having trouble with Running bc script multiply.bc... taking forever. seems like it's actually the diff that takes forever. using toybox diff on a clean git clone of your upstream on the host, make test passes fine in a reasonable time, so it's not that.

oh, yeah, pulling the files-to-be-diffed off the device, they're pretty different:

enh-p920.mtv:~/aosp$ wc -l multiply*
  3238615 multiply_script_results.txt
  2786743 multiply.txt
  6025358 total
enh-p920.mtv:~/aosp$ diff -u multiply.txt multiply_script_results.txt | wc -l
3128717
enh-p920.mtv:~/aosp$ 

seems like after a certain point i have a line missing every 5 lines?

--- multiply.txt        2021-08-11 17:09:34.017797251 -0700
+++ multiply_script_results.txt 2021-08-11 17:10:41.998168038 -0700
@@ -38,8 +38,7 @@
 617283945000000000000000000000000000000000000.00000000000000000000
 6172839450000000000000000000000000000000000000.00000000000000000000
 61728394500000000000000000000000000000000000000.00000000000000000000
-617283945000000000000000000000000000000000000000.0000000000000000000\
-0
+617283945000000000000000000000000000000000000000.00000000000000000000
 6172839450000000000000000000000000000000000000000.000000000000000000\
 00
 61728394500000000000000000000000000000000000000000.00000000000000000\
@@ -59,143 +58,175 @@
 617283945000000000000000000000000000000000000000000000000.0000000000\
 0000000000
 3810394687547630.25000000000000000000
+38103946875476302.50000000000000000000
 3810394687547630.25000000000000000000
 3810394687547630.25000000000000000000
 3810394687547630.25000000000000000000
 38103946875476302.50000000000000000000
+381039468754763025.00000000000000000000
 38103946875476302.50000000000000000000
 38103946875476302.50000000000000000000
 38103946875476302.50000000000000000000
 381039468754763025.00000000000000000000
+3810394687547630250.00000000000000000000
 381039468754763025.00000000000000000000
 381039468754763025.00000000000000000000
 381039468754763025.00000000000000000000
 3810394687547630250.00000000000000000000

i don't suppose that looks at all familiar?

i have already tried the obvious thing of adding all the extra -Ds from your build to mine:

+"-DBC_NUM_KARATSUBA_LEN=32",
+"-DBC_ENABLE_MEMCHECK=0",
+"-DBC_ENABLE_AFL=0",
+"-DBC_DEFAULT_BANNER=0",
+"-DBC_DEFAULT_SIGINT_RESET=1",
+"-DBC_DEFAULT_TTY_MODE",
+"-DBC_DEFAULT_PROMPT=1",

[gavinhoward]

just to confirm: you don't know of any problems they've had, you're just waiting to see if they have any?

They definitely have problems, but they are trying to figure out if they need to fix it or if I need to.

With the multiply script, I know what's happening. You don't need all of the extra -Ds.

In 5.0.0, I made a change where if there is one more digit to print, and space for only one more digit on the line, it prints the digit instead of a backslash+newline+digit combo. I did this because OpenBSD does, and after more carefully reading the bc spec, I felt like that was the correct behavior.

This, however, introduced a problem for the generated tests because GNU bc does not do that. So as part of generating tests, I needed some way of fixing GNU bc's output.

I did this with tests/script.sed and piping the generated test through sed before printing it to a file. (See https://github.com/gavinhoward/bc/blob/master/tests/script.sh#L155-L161 and https://github.com/gavinhoward/bc/blob/master/manuals/development.md#script-tests .)

To fix the problem on your end, I would reimport all of the tests/ directory, especially since I added some new tests for the new functions in gen/lib2.bc. Oh, and on that note, I would import the gen/ directory too.

Other than that, the multiply script does take a longer time now because I expanded the tests to catch some cases it didn't before because I thought that my bc might have a bug there. (It didn't, thank goodness.)

If I were you, though, I would actually turn off generated tests. However, it also looks like you do? run-bc-tests-on-android.sh appears to turn off generated tests, which should mean that the multiply script test is not run, so I am confused...

With all of that said, what would you like me to do?

Side note: you can also take -DBC_ENABLE_LONG_OPTIONS and -DBC_ENABLE_SIGNALS out of Android.bp; they haven't been used for a while.

[enh-google]

so I am confused

yeah, me too, but you gave me enough clues for the lightbulb to come on :-)

i didn't know what you meant by "reimport", since i'd already taken everything ... which made me check your .gitignore file ... a little editing of which led to this in git status:

Untracked files:
  (use "git add <file>..." to include in what will be committed)
    multiply_script_results.txt
    tests/bc/scripts/add.txt
    tests/bc/scripts/divide.txt
    tests/bc/scripts/multiply.txt
    tests/bc/scripts/subtract.txt
    tests/bc_outputs/

so i think what happened here was that i probably ran your make test in my AOSP tree at some point. so when i run locally i have those (now outdated) files lying around, but everyone else (and the build servers) don't. anyway, a bit of local rming later (and removing the unnecessary -Ds, including the couple of already checked-in ones you pointed out), and...

...
Running bc stdin tests...pass
Skipping bc script multiply.bc
Skipping bc script divide.bc
Skipping bc script subtract.bc
Skipping bc script add.bc
Running bc script print.bc...pass
...
Running bc limits tests...pass

All bc tests passed.

***********************************************************************

so i think that mystery's solved. i would definitely benefit by always doing every new bit of work in a completely fresh AOSP checkout, but it takes far too long to do that that even with the odd wasted day here and there i'm probably still coming out ahead with all these weird surprises!

With all of that said, what would you like me to do?

for the -D stuff? let me check in the fix needed in run-bc-tests-on-android.sh and then i'll try your new patch and get back to you...

on the other hand if you meant "to help debug your upgrade issue", you already did that, thanks! :-)

[enh-google]

However, I think I found a solution: 7e21816. Since include/status.h includes include/bcl.h after that point, I can use it to disable building the library by default when BC_ENABLE_LIBRARY is not defined.

Can you test that 7e21816 fixes your problem? If so, I will make another release, though I may wait on any problems FreeBSD has with updating their in-tree version.

yeah, that works great. i'll remove the Android.bp hack in the next update. (i'll add a TODO right now so i don't forget, and remove the other bits i don't need at the same time too!)

[gavinhoward]

Good to know! Closing....

[enh-google]

heh, you got 5.0.1 out so fast i've skipped the TODO stage and just fixed it directly in the update!

thanks again for the help! pleasure working with you as always :-)