On Fri, Jul 22, 2016 at 12:45 AM, Rob Landley <[email protected]> wrote: > On 07/21/2016 12:33 AM, Andy Chu wrote: >> expr is in pending, but ships on Android. > > Yes, that is a significant issue I need to address. However, "issue" > isn't necessarily "the real problem". The _problem_ is I have many > things to do and a finite amount of time/energy to do them, but I'm > working on it. > > Right now I'm trying to get grep to where android can use it, and get dd > cleaned up and promoted out of pending. After that expr is near the top > of the list. > > When you say "a thing in pending is broken" I tend to go "yes, that's > why it's in pending". When you say "prioritize this thing in pending" > what I prioritize is getting it out of pending.
Personally, my concern isn't that expr is in pending. My concern is that it has a bug that I provided a patch for 4 months ago (which is evident with either code inspection or by running ASAN, and which I reminded you about once). If it's shipping, the priority should be that it works, not what directory it's in. I realize that taking code out of pending can be a long process, and is mostly about being reviewed and that you understand and can support all of it. But applying patches *with tests*, where verification of the fix doesn't require mental simulation of the code, should be fast. (Not to mention it's a 10 line patch.) That's actually one of the motivations for having sanitizers in the tree. So it is *dirt simple* to tell that I actually fixed something! To me, correctness is the prerequisite for readability/maintainability. First you make it correct, and test it so it STAYS correct (this is hindered by the poor shape of the test suite). And only then can you safely refactor and optimize. This is my opinion -- if refactoring is so important that you don't care what kind of bugs you introduce in the process, so be it... patch: http://lists.landley.net/pipermail/toybox-landley.net/2016-March/008113.html bug: http://lists.landley.net/pipermail/toybox-landley.net/2016-March/008140.html > You instead want to do a major redesign of the testing directory based > on this week's coverity/valgrind variant, which is a different area This is a misconception... you either never looked at what I did, or you forgot. Patch 4/4 was the one which added ASAN. Yes you did rewrite and apply some of the first patch or so, but I'm pretty sure some of the problems remain. To jog our memories I will copy the patch descriptions at the end of this message. OK now that I look about it I'm pretty sure the host/toybox distinction remains unfixed, as well as the exit codes (meaning it's unclear if the tests passed or failed), and possibly some other stuff. I agree with the entire list of things that you posted and my patches made progress toward those. I'm not sure why you think otherwise, other than the barrier of understanding someone else's code. The test harness isn't even clear about the test environment. For example, which binaries are in PATH differs when you're running a single command test vs the entire suite, which means the RESULTS differ. Getting that VERY BASIC detail right is a prerequisite for doing testing in a VM. > I'm working as fast as I can, but I don't necessarily have the same > priorities you do. The ideal would be if contributions actually sped up the project rather than slowed it down. I'm not sending the patches in hopes of putting stuff on your TODO list. I'm sending them so that work can be parallelized and the project can make quicker progress, i.e. LESS stuff on your TODO list. In particular I was sending them so that I have a decent testing environment in which to implement other commands. I also believe that better tests will make many patches easier and faster to review, but I don't think you really work that way unfortunately. Andy PATCH DESCRIPTIONS: Date: Fri, 18 Mar 2016 18:02:17 -0700 Subject: [PATCH 1/2] Refactor the test harness to triage tests, and fix some bugs. genconfig.sh: - Fix a name clash bug where 'make test' would run all tests and then try to build the 'test' binary. 'make test_bin' is special cased for now. - Make the test targets .PHONY, since we're not creating files called test_sed, etc. - Use here docs to write Makefile fragments, for readability and to be consistent with the rest of the script. Refactor into functions. - Add comments. make.sh: - Add the output binary name as the first param. This fixes an issue where 'make; make test_sed' would leave you without a 'toybox' binary because single.sh created another toybox binary and renamed it to sed. runtest.sh: - Changed the 'testing' function to return 0. This fixes a bug where the return value of the script depends on the return code of the last command -- NOT whether it passed or failed. For example, 'make test_test' would fail with code 1, even though all tests passed (because 'test' exits 1 validly). On the other hand, tests with failures would still return 0 if the last command didn't fail. - Use local variables instead of globals since we are sharing globals with the tests themselves. - Add a common function to skip tests if not running as root, and keep track of skipped commands. test.sh: - Refactor it into separate functions/actions and add usage. - Fix a bug where tests would succeed under 'make test_$COMMAND' but fail under 'make test'. The problem is if you are running the xzcat command, 'tar' could be either the host binary or the toybox binary, depending on what tools are in the modified test $PATH. To fix this, the test harness finds the host binary and sets $HOST_BIN_TAR, etc. before invoking the test. - Keep track of the number of failures per *command*, and exit 1 if any failed - Use a distinct test directory for each command, instead of repeatedly setting up and tearing down the same one. - When running all tests, print out a summary at the end. single.sh: - Call scripts/make.sh directly instead of 'make'. - Add comments. Subject: [PATCH 2/2] Various test fixes and cleanup. bzcat: Fix a test that was succeeding for the wrong reason. The test was looking for a 1 exit code in the case of bad data, but there was a typo in the input path, so it failed 1 for the wrong reason. bzcat, xzcat, zcat: Use $HOST_BIN_TAR to fix the issue where they fail under 'make test' but not 'make test_bzcat'. hostname: Use $HOST_BIN_HOSTNAME, and skip one test if not root. chgrp, groupadd, groupdel, ifconfig, losetup, mount, useradd: Skip if not root. pgrep, pkill: style cleanup and speedup of the tests (change sleep 1 to sleep 0.1). No fixes yet. renice: Fix some quoting problems (revealing further errors, not fixed) tar: Style cleanup in preparation to fix failures under 'make test' but not 'make test_tar' (due to host/toybox discrepancy) touch: Use $HOST_BIN_DATE because toybox doesn't support %N nanoseconds Subject: [PATCH 3/3] Small fix to make.sh: use OUTNAME instead of toybox Subject: [PATCH 4/4] Build and test changes to support Clang sanitizers. ./test.sh is a new "front end" for running tests in different modes. I originally wrote this patch mostly in make, so you would do 'make asantest_sed', analogous to 'make test_sed'. But .singlemake became much bigger, and really it's silly to auto-generate multiple targets for every command when you can just have a shell script that takes the command as an argument. So now the interface is: ./test.sh all 'make test' is an alias ./test.sh single sed 'make test_sed' is an alias Run ./test.sh without args for details and examples. See the comment at the top of the Makefile for the tree layout. Details: Makefile: Add toybox_asan, toybox_msan, toybox_ubsan targets. make.sh: - Allow outputting only the unstripped binary, as toybox_asan_unstripped would never be useful - Create different obj/ dirs based on CC and CFLAGS. This reduces the need to 'make clean' a lot. (Each of the 3 sanitizers does different, incompatible runtime instrumentation.) single.sh: Generate single tools in generated/single/ rather than the root directory. This prevents us from having to special case 'test' in a lot of places, and also 'make clean' no longer needs code generation. ./test.sh: Fix the host/toybox divergence between the environment in 'make test' and 'make test_$COMMAND'. The latter only had one toybox binary in the PATH, so everything would fall back to host binaries. scripts/test.sh: This is a helper tool now, not to be invoked by the developer. ./test.sh is the front end -- it automatically builds dependent binaries make symlinks, and then invokes scripts/test.sh. Prior to this change you had to run 'make' before 'make test', now you can just to 'make test' from a clean tree if you like. _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
