Closed enh-google closed 1 year ago
i haven't been able to update for some time (5.0.1 was our last update!)
Oof...I'm sorry. Have I been missing communication with you?
because the tests fail in a weird way without much useful information. i've included the current failures for 6.2.2 below, for example.
I agree; that is useless.
my guess is that this is something to do with Android using mksh as its /bin/sh. the interleaved output makes it appear like there might be some disagreement about job handling between bash and mksh?
I think that's a great guess, and it's my guess too because the tests/all.sh
script does use job control. I tried to write it according to the POSIX standard, but I suspect that job control is finicky in general.
are the current bc tests known to work on shells other than bash (and in particular mksh)? any ideas where i should look (or some kind of parallelism i can just disable)?
They are known to work on tcsh
(FreeBSD), but other than that, bash
, and dash
, I don't know.
any ideas where i should look (or some kind of parallelism i can just disable)?
To disable parallelism, change the last line of run-bc-tests-on-android.sh
to:
exec adb shell $dash_t /data/local/tmp/bc-tests/tests/all.sh -n bc 0 1 0 0 0 bc
I made two changes:
-n
option, which is how to disable parallelism in the tests/all.sh
script.bc
because there are now five integer positional arguments.My hope (and it is only a hope) is that that missing integer argument is what is causing the weird failures because the script would otherwise treat the last bc
argument as the last integer argument and cause some sort of failure. bash
fails because it has a non-integer argument, but I don't know how mksh
would fail in that case.
The other reason there might be failures is because I added a test that is "problematic." It's my test for malloc()
failure, and unfortunately, it only works on Linux systems with swap disabled and some other changes. I don't think it would work on Android. I did add an option to disable that test in tests/all.sh
; that is the missing integer option, in fact. But I don't think that's the reason because I added it in the fourth position (the last is whether to time the tests), so it is a 0 in your current script.
Regardless, with the parallelism disabled, you should see exactly what test is failing, and you should be able to give me that output, even if either of those are not the reason for the failure.
In the meantime, I'll work on my end. Android is a first-class user of my bc
, and I need to start treating it that way, including setting up a building and testing environment. I'll do my best to do so myself, but if possible, can you give me a helpful link?
i haven't been able to update for some time (5.0.1 was our last update!)
Oof...I'm sorry. Have I been missing communication with you?
no, this is just the first time i've had time to actually look at the problem and file a bug!
are the current bc tests known to work on shells other than bash (and in particular mksh)? any ideas where i should look (or some kind of parallelism i can just disable)?
They are known to work on
tcsh
(FreeBSD), but other than that,bash
, anddash
, I don't know.
yeah, there's been a lot about job control on the POSIX mailing list recently too.
any ideas where i should look (or some kind of parallelism i can just disable)?
To disable parallelism, change the last line of
run-bc-tests-on-android.sh
to:exec adb shell $dash_t /data/local/tmp/bc-tests/tests/all.sh -n bc 0 1 0 0 0 bc
yes, that works. thanks!
In the meantime, I'll work on my end. Android is a first-class user of my
bc
, and I need to start treating it that way, including setting up a building and testing environment. I'll do my best to do so myself, but if possible, can you give me a helpful link?
there's https://source.android.com/docs/setup/build/building (and other pages about running), but it's a pretty large undertaking.
tbh, i've only had two problems with bc updates:
-n
when you make scary changes" but one thing you haven't done but could that i think would help would be to fail hard --- making extra options optional (ho ho) makes my life harder. an error message would be more helpful, especially because changes to a row of 0s and 1s aren't necessarily immediately obvious!anyway, https://android-review.googlesource.com/c/platform/external/bc/+/2385321 works for me locally, so it should make it through CI. (and if it doesn't, that's probably a question for me anyway :-) )
thanks!
there's https://source.android.com/docs/setup/build/building (and other pages about running), but it's a pretty large undertaking.
I'll do my best anyway. Thank you!
tbh, i've only had two problems with bc updates
- build changes such as the move from a shell script to a .c file for strgen. those are almost certainly easier for me to do anyway.
- test changes like this one. these have been harder for me to adapt to (although this is the first one i've failed with). there i'd say "yes, please make sure you add options like -n when you make scary changes" but one thing you haven't done but could that i think would help would be to fail hard --- making extra options optional (ho ho) makes my life harder. an error message would be more helpful, especially because changes to a row of 0s and 1s aren't necessarily immediately obvious!
I think both of these are oversights on my part. I should add sections to my NEWS.md
entry for each version about building, packaging, and testing changes. I've never thought they were necessary to list, but here is hard evidence that yes, they are.
I apologize. I will do that going forward. And I also apologize for accidentally using you as my test subject while I learn how to run a project.
Now, about making things fail hard: I agree that I messed up by making arguments optional.
However, because I have downstream users that could depend on the options staying optional, I'm not sure I can make them required at this point.
Nevertheless, I think I can do the next best thing: do error checking on each argument. This would (hopefully) allow me to add the "fail hard" you want (and it's a good idea!) while not causing problems for other downstream users. (Unless they have bugs in their scripts, in which case, they should be fixed anyway.)
I have implemented the ideas in d5e6dbdf328407bddf53efb655abe4b9c2fcb90f, so they'll be in the next release. I hope that helps.
I apologize. I will do that going forward. And I also apologize for accidentally using you as my test subject while I learn how to run a project.
no worries --- we're a bit of an outlier anyway. it's only when you get included in a "ridiculously large" meta-project (most commonly an operating system [or moral equivalent] such as android or chromium or whatever) that you tend to find people not using your build system because the pain of having to duplicate your build system is still slightly less than the pain of trying to incorporate your build system into theirs!
Now, about making things fail hard: I agree that I messed up by making arguments optional.
However, because I have downstream users that could depend on the options staying optional, I'm not sure I can make them required at this point.
Nevertheless, I think I can do the next best thing: do error checking on each argument. This would (hopefully) allow me to add the "fail hard" you want (and it's a good idea!) while not causing problems for other downstream users. (Unless they have bugs in their scripts, in which case, they should be fixed anyway.)
yeah, maybe if you ever need more arguments, make them named? (like --foo=bar
or foo=bar
?)
I have implemented the ideas in d5e6dbd, so they'll be in the next release. I hope that helps.
thanks. since you're obviously interested in the feedback, i'll try to let you know sooner next time i have an update where manual intervention is required, if i have any trouble :-)
i haven't been able to update for some time (5.0.1 was our last update!) because the tests fail in a weird way without much useful information. i've included the current failures for 6.2.2 below, for example.
my guess is that this is something to do with Android using mksh as its /bin/sh. the interleaved output makes it appear like there might be some disagreement about job handling between bash and mksh?
are the current bc tests known to work on shells other than bash (and in particular mksh)? any ideas where i should look (or some kind of parallelism i can just disable)?