Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
On Mon 2019-06-24 21:44:13 +0300, Tomi Ollila wrote: > If this suite speedup is merged, I'd suggest using original David's patch > 2/2 due to consistency reasons -- $gen_test_filename is used likewise in > other test_expect_code cases. and, unless $TEST_DIRECTORY contains '"'s, > '$'s or '`'s it work (in our current cases $gen_test_name does not contain > the above characters (gen_test_filename=$TEST_DIRECTORY/$gen_test_name)) > > I've looked this quite a lot lately (mostly for fun). I'll send email in > near future some suggestions (ten (10) or so) how we could improve the > situation here, which then could be applied everywhere... If you insist, i'm ok to wait on your cleanup for pathnames that have unusual characters in them, though i'd also be happy to see the cleanup applied to make *inconsistent* things consistent, rather than enforcing a broken-yet-uniform consistency and then applying the cleanup. But we should not use Bremner's patch 2/2 directly, because it moves shim generation below gen_insert_msg, as mentioned in id:8736kipe9f@fifthhorseman.net. --dkg signature.asc Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
* Daniel Kahn Gillmor: > Do you think we could go ahead and apply these patches on master now > anyway, and fix them up subsequently to make sure they apply to MacOS? I still have not been able to set up any macOS based tests, because contractual work has kept me busy. I suggest you go ahead with the changes, and should I run into any unexpected issues in future builds, I'll speak up. I don't want to slow you folks down. -Ralph ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
On Mon, Jun 24 2019, Daniel Kahn Gillmor wrote: > On Fri 2019-06-14 08:16:14 -0300, David Bremner wrote: >> Ralph Seichter writes: >> >>> * Daniel Kahn Gillmor: >>> Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how it'll affect homebrew? >>> >>> MacPorts, actually. ;-) I have not yet been able to look into this patch >>> series, but I hope to be able to do so soonish. >>> >>> -Ralph >> >> Supposedly DYLD_INSERT_LIBRARIES does (did?) the same job as LD_PRELOAD, >> but there seems to some macOS specific complications. > > Do you think we could go ahead and apply these patches on master now > anyway, and fix them up subsequently to make sure they apply to MacOS? > I don't know what the "MacOS specific complications" are. Can they be > spelled out in more detail? I note that > https://stackoverflow.com/questions/34114587/dyld-library-path-dyld-insert-libraries-not-working > suggests that DYLD_INSERT_LIBRARIES won't work with signed binaries, but > i don't think the binaries tested during the test suite are signed > binaries, are they? > > Alternately if DYLD_INSERT_LIBRARIES doesn't work, should we just skip > this test on MacOS? or have it fall back to gdb? > > I'd really like to see this test suite speedup merged. If this suite speedup is merged, I'd suggest using original David's patch 2/2 due to consistency reasons -- $gen_test_filename is used likewise in other test_expect_code cases. and, unless $TEST_DIRECTORY contains '"'s, '$'s or '`'s it work (in our current cases $gen_test_name does not contain the above characters (gen_test_filename=$TEST_DIRECTORY/$gen_test_name)) I've looked this quite a lot lately (mostly for fun). I'll send email in near future some suggestions (ten (10) or so) how we could improve the situation here, which then could be applied everywhere... > > --dkg Tomi PS: actually, there is e.g. T070-insert.sh:test_expect_code 1 "notmuch insert --folder=nonesuch < $gen_msg_filename" (and that is not the only one) In this case also ' ', \t and \n in $gen_msg_filename would make tests break (e.g. someone runs tests in $PWD that has spaces in it). ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
On Fri 2019-06-14 08:16:14 -0300, David Bremner wrote: > Ralph Seichter writes: > >> * Daniel Kahn Gillmor: >> >>> Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how >>> it'll affect homebrew? >> >> MacPorts, actually. ;-) I have not yet been able to look into this patch >> series, but I hope to be able to do so soonish. >> >> -Ralph > > Supposedly DYLD_INSERT_LIBRARIES does (did?) the same job as LD_PRELOAD, > but there seems to some macOS specific complications. Do you think we could go ahead and apply these patches on master now anyway, and fix them up subsequently to make sure they apply to MacOS? I don't know what the "MacOS specific complications" are. Can they be spelled out in more detail? I note that https://stackoverflow.com/questions/34114587/dyld-library-path-dyld-insert-libraries-not-working suggests that DYLD_INSERT_LIBRARIES won't work with signed binaries, but i don't think the binaries tested during the test suite are signed binaries, are they? Alternately if DYLD_INSERT_LIBRARIES doesn't work, should we just skip this test on MacOS? or have it fall back to gdb? I'd really like to see this test suite speedup merged. --dkg ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
Ralph Seichter writes: > * Daniel Kahn Gillmor: > >> Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how >> it'll affect homebrew? > > MacPorts, actually. ;-) I have not yet been able to look into this patch > series, but I hope to be able to do so soonish. > > -Ralph Supposedly DYLD_INSERT_LIBRARIES does (did?) the same job as LD_PRELOAD, but there seems to some macOS specific complications. d ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
* Daniel Kahn Gillmor: > Perhaps Ralph Seichter (explicitly cc'ed above) could comment on how > it'll affect homebrew? MacPorts, actually. ;-) I have not yet been able to look into this patch series, but I hope to be able to do so soonish. -Ralph ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
On Sun 2019-05-26 10:08:54 -0300, David Bremner wrote:
> This removes the dependency of this test script on gdb, and
> considerably speeds up the running of the tests.
This series looks good to me. I've tested it with moreutils parallel
installed, and it reduces total CPU time for the parallel test suite
from ~76 seconds of CPU to ~56 seconds, a > %25 reduction in CPU cost
for the whole test suite, despite touching only one test. This is
awesome work.
I also like the elegance of the proposed change. It would be great to
see it extended to the rest of our test suite's dependency on GDB if we
can do it. (T050, T060, and T380 i think)
Details on my profiling:
On the current master (bc396c967c7cd8e7a109858e428d7bf97173f7a7),
without these changes applied, the whole test suite consumes this much
CPU on my 4-core intel i5-2540M (2.60GHz):
real0m33.106s
user1m1.150s
sys 0m14.998s
On the same machine, with the pair of patches applied, the whole test
suite consumes this:
real0m27.557s
user0m44.172s
sys 0m12.018s
Caveat: i don't know how well LD_PRELOAD works on non-GNU/Linux
platforms, and we're not using LD_PRELOAD elsewhere in the test suite
yet. Perhaps Ralph Seichter (explicitly cc'ed above) could comment on
how it'll affect homebrew? Do we need to consider a variant for
platforms that can't do LD_PRELOAD?
One nit-pick below:
> diff --git a/test/T070-insert.sh b/test/T070-insert.sh
> index 48165caa..ab26ecd4 100755
> --- a/test/T070-insert.sh
> +++ b/test/T070-insert.sh
> @@ -2,8 +2,6 @@
> test_description='"notmuch insert"'
> . $(dirname "$0")/test-lib.sh || exit 1
>
> -test_require_external_prereq gdb
> -
> # subtests about file permissions assume that we're working with umask
> # 022 by default.
> umask 022
> @@ -246,50 +244,38 @@ test_expect_code 1 "notmuch insert $gen_msg_filename
> 2>&1"
> notmuch config set new.tags $OLDCONFIG
>
> # DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
> +gen_insert_msg
>
> -for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
> -READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR; do
> -cat < index-file-$code.gdb
> -set breakpoint pending on
> -set logging file index-file-$code.log
> -set logging on
> -break notmuch_database_index_file
> -commands
> -return NOTMUCH_STATUS_$code
> -continue
> -end
> -run
> +# pregenerate all of the test shims
> +for code in FILE_NOT_EMAIL READ_ONLY_DATABASE UPGRADE_REQUIRED PATH_ERROR
> OUT_OF_MEMORY XAPIAN_EXCEPTION; do
> +make_shim shim-$code < +#include
> +#include
> +notmuch_status_t
> +notmuch_database_index_file (notmuch_database_t *notmuch,
> + const char *filename,
> + notmuch_indexopts_t *indexopts,
> + notmuch_message_t **message_ret)
> +{
> + return NOTMUCH_STATUS_$code;
> +}
> EOF
> done
>
> -gen_insert_msg
> -
Why put the shim generation below gen_insert_msg? If it were above, it
would make the comment about DUPLICATE_MESSAGE_ID less confusing, and
the changeset could be shorter.
--dkg
signature.asc
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 2/2] test: replace use of gdb with LD_PRELOAD shims in T070-insert.sh
One more nit-pick: On Sun 2019-05-26 10:08:54 -0300, David Bremner wrote: > +test_expect_code 0 "notmuch_with_shim shim-$code insert --keep < > \"$gen_msg_filename\"" This kind of business breaks obscurely if $gen_msg_filename happens to have U+0022 QUOTATION MARK in it. That's a pretty perverse situation, but i'd generally prefer to use bash's builtin printf's %q for robustness. A revised patch will follow shortly that fixes both of my nitpicks. --dkg signature.asc Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
