|
From: Bart V. A. <bar...@gm...> - 2009-02-19 10:15:01
|
On Thu, Feb 19, 2009 at 10:52 AM, <sv...@va...> wrote: > - A number of tests are shared between Helgrind and DRD. They used to be > built in both directories. Now they are only built in helgrind/tests/, > and the DRD .vgtest files just point to the executable in helgrind/tests/. > Most of these (about 30) had the source files in helgrind/tests/; I moved > the three that were in drd/tests/ into helgrind/tests/ for consistency. Shouldn't such changes be discussed on the valgrind-developers mailing list before being implemented ? Bart. |
|
From: Nicholas N. <n.n...@gm...> - 2009-02-19 10:33:14
|
On Thu, Feb 19, 2009 at 9:14 PM, Bart Van Assche <bar...@gm...> wrote: > On Thu, Feb 19, 2009 at 10:52 AM, <sv...@va...> wrote: >> - A number of tests are shared between Helgrind and DRD. They used to be >> built in both directories. Now they are only built in helgrind/tests/, >> and the DRD .vgtest files just point to the executable in helgrind/tests/. >> Most of these (about 30) had the source files in helgrind/tests/; I moved >> the three that were in drd/tests/ into helgrind/tests/ for consistency. > > Shouldn't such changes be discussed on the valgrind-developers mailing > list before being implemented ? It seemed like a clear simplification to me and thus not controversial. What are your objections? Nick |
|
From: Bart V. A. <bar...@gm...> - 2009-02-19 10:51:12
|
On Thu, Feb 19, 2009 at 11:33 AM, Nicholas Nethercote <n.n...@gm...> wrote: > On Thu, Feb 19, 2009 at 9:14 PM, Bart Van Assche > <bar...@gm...> wrote: >> On Thu, Feb 19, 2009 at 10:52 AM, <sv...@va...> wrote: >>> - A number of tests are shared between Helgrind and DRD. They used to be >>> built in both directories. Now they are only built in helgrind/tests/, >>> and the DRD .vgtest files just point to the executable in helgrind/tests/. >>> Most of these (about 30) had the source files in helgrind/tests/; I moved >>> the three that were in drd/tests/ into helgrind/tests/ for consistency. >> >> Shouldn't such changes be discussed on the valgrind-developers mailing >> list before being implemented ? > > It seemed like a clear simplification to me and thus not > controversial. What are your objections? I do see it this way -- I do not agree that the DRD regression tests have been moved to another directory. I want to see these back in the drd directory. Bart. |
|
From: Nicholas N. <n.n...@gm...> - 2009-02-19 11:25:49
|
On Thu, Feb 19, 2009 at 9:51 PM, Bart Van Assche <bar...@gm...> wrote: > > I do see it this way -- I do not agree that the DRD regression tests > have been moved to another directory. I want to see these back in the > drd directory. So you want pth_barrier.c, rwlock_race.c and rwlock_test.c to be moved back into drd/tests/? I think it's simpler to have dependencies in one direction rather than in both directions. But if it really concerns you, I can do it tomorrow morning -- I'm about to go to bed now. Nick |
|
From: Bart V. A. <bar...@gm...> - 2009-02-19 13:36:14
|
On Thu, Feb 19, 2009 at 12:25 PM, Nicholas Nethercote <n.n...@gm...> wrote: > On Thu, Feb 19, 2009 at 9:51 PM, Bart Van Assche > <bar...@gm...> wrote: >> >> I do see it this way -- I do not agree that the DRD regression tests >> have been moved to another directory. I want to see these back in the >> drd directory. > > So you want pth_barrier.c, rwlock_race.c and rwlock_test.c to be moved > back into drd/tests/? Yes. There are several reasons why I want to see these tests being moved back to the drd/tests directory: - This makes it clear who spent the effort to write the test -- currently the only way to know who wrote a Helgrind or Drd regression test is to look up in which directory the source code of the test resides. - Before I started using some of the Helgrind regression tests for testing Drd, I asked Julian for permission through private e-mail. Julian asked me not to copy the source code of the Helgrind regression tests but to create soft links from helgrind/tests/ to drd/tests/. I choose to specify the ../../helgrind/tests path explicitly in drd/tests/Makefile.am. I assume that Julian is using some of the Drd regression tests under the same conditions. - The reason why I choose to recompile the Helgrind regression tests in the drd/tests/ directory instead of directly running the helgrind/tests/... binaries during testing Drd is that this decouples the regression tests of both tools better: this way the CFLAGS specified in drd/tests/Makefile.am apply to all binaries used for regression testing Drd. The latest trunk revision (r9202) some binaries that are being used to test Drd have been compiled with the CFLAGS specified in drd/tests/Makefile.am and some have been compiled with the CFLAGS specified in helgrind/tests/Makefile.am. IMHO the above discussion should have been held before any changes were applied to the Helgrind or Drd regression tests. > But if it really concerns you, I can do it tomorrow morning -- I'm > about to go to bed now. Thanks, Bart. |
|
From: Nicholas N. <n.n...@gm...> - 2009-02-19 23:26:56
|
On Thu, Feb 19, 2009 at 10:57 PM, Bart Van Assche
<bar...@gm...> wrote:
>
> Yes. There are several reasons why I want to see these tests being
> moved back to the drd/tests directory:
> - This makes it clear who spent the effort to write the test --
> currently the only way to know who wrote a Helgrind or Drd regression
> test is to look up in which directory the source code of the test
> resides.
I didn't expect the ownership of 3 simple test files, a total of 218
lines of code, to be an issue. None of our test files have the author
indicated, so for most of them the author is unclear.
> - Before I started using some of the Helgrind regression tests for
> testing Drd, I asked Julian for permission through private e-mail.
> Julian asked me not to copy the source code of the Helgrind regression
> tests but to create soft links from helgrind/tests/ to drd/tests/. I
> choose to specify the ../../helgrind/tests path explicitly in
> drd/tests/Makefile.am. I assume that Julian is using some of the Drd
> regression tests under the same conditions.
I believe Julian asked you to do that for a purely technical reason:
to avoid the code duplication. We do the same thing with the
{none,memcheck}/tests/x86/insn* tests.
> - The reason why I choose to recompile the Helgrind regression tests
> in the drd/tests/ directory instead of directly running the
> helgrind/tests/... binaries during testing Drd is that this decouples
> the regression tests of both tools better: this way the CFLAGS
> specified in drd/tests/Makefile.am apply to all binaries used for
> regression testing Drd. The latest trunk revision (r9202) some
> binaries that are being used to test Drd have been compiled with the
> CFLAGS specified in drd/tests/Makefile.am and some have been compiled
> with the CFLAGS specified in helgrind/tests/Makefile.am.
It's true that the CFLAGS are different. However, the only difference
is that DRD uses different -W flags, so the executables are actually
identical. I actually think the fact that DRD uses different -W flags
for its tests compared to every other tool is actually a bad thing,
I'd like to get rid of that difference.
(And if you're going to argue that the old approach allowed other
compilation changes, should they be needed:
http://en.wikipedia.org/wiki/You_Ain't_Gonna_Need_It. I again point
to the insn* tests which have been happily shared between Nulgrind and
Memcheck for years.)
> IMHO the above discussion should have been held before any changes
> were applied to the Helgrind or Drd regression tests.
Our usual procedure, which admittedly isn't explicitly documented, is
that developers don't bother discussing things before committing
unless they think they are controversial. Then if someone else
doesn't like it, they can complain in response to the log message.
This is a lightweight review process appropriate for a project like
Valgrind with a small number of developers. And that's exactly what
has happened here, which is fine. Obviously people will sometimes
disagree about the definition of controversial, and I'm sorry if this
process wasn't clear and I've upset you. But after all, it's just the
build system and the tests... I'd be much more careful about changing
DRD's code, for example, or even changing the contents of the tests,
but this change just moved them around and changed how they were
built.
Our build system is something of a mess and I've put quite a lot of
effort recently into improving it. My change reduced the size of
drd/tests/Makefile.am by 205 lines, made "make check" faster by
avoiding building 35 tests twice, and simplified the dependencies
between Helgrind and DRD. So I'd rather not revert it.
I'd be happy for you to put a comment in the tests you wrote
indicating that you are the author (we could do that for all the tests
you've written if you'd like.) If you're still not happy with that,
how's this for a compromise that I expect Julian won't mind: how
about we move all the shared test source files from helgrind/tests/
into drd/tests/ ?
Nick
|
|
From: Bart V. A. <bar...@gm...> - 2009-02-20 19:07:06
|
On Fri, Feb 20, 2009 at 12:00 AM, Nicholas Nethercote <n.n...@gm...> wrote: > It's true that the CFLAGS are different. However, the only difference > is that DRD uses different -W flags, so the executables are actually > identical. I actually think the fact that DRD uses different -W flags > for its tests compared to every other tool is actually a bad thing, > I'd like to get rid of that difference. There is a very good reason why I added -Wextra to the CFLAGS in drd/tests/Makefile.am and not in the other Makefile.am files. Just like any software, test programs can contain bugs. Flags like -Wextra help to catch these bugs as early as possible. I did not add this flag to other Makefile.am files because it would take a lot of work to get the rest of the source code in a state such that it compiles cleanly with -Wextra. Bart. |
|
From: Bart V. A. <bar...@gm...> - 2009-02-20 07:11:13
|
On Fri, Feb 20, 2009 at 12:00 AM, Nicholas Nethercote <n.n...@gm...> wrote: > If you're still not happy with that, > how's this for a compromise that I expect Julian won't mind: how > about we move all the shared test source files from helgrind/tests/ > into drd/tests/ ? Please move the tests that you moved away from drd/tests/ back to drd/tests/. If you don't do this, I will do it. Bart. |
|
From: Nicholas N. <n.n...@gm...> - 2009-02-20 07:36:35
|
On Fri, Feb 20, 2009 at 6:11 PM, Bart Van Assche <bar...@gm...> wrote: > On Fri, Feb 20, 2009 at 12:00 AM, Nicholas Nethercote > <n.n...@gm...> wrote: >> If you're still not happy with that, >> how's this for a compromise that I expect Julian won't mind: how >> about we move all the shared test source files from helgrind/tests/ >> into drd/tests/ ? > > Please move the tests that you moved away from drd/tests/ back to > drd/tests/. If you don't do this, I will do it. It appears your main concern is over the perceived authorship/ownership of the test files. So, fine, go ahead and move the source files back. But I don't want everything to be built twice or the other simplifications I made to be rolled back. Nick |
|
From: Bart V. A. <bar...@gm...> - 2009-02-20 08:03:04
|
On Fri, Feb 20, 2009 at 8:36 AM, Nicholas Nethercote <n.n...@gm...> wrote: > But I don't want everything to be built twice > or the other simplifications I made to be rolled back. I didn't even start looking at the other changes. If I have any comments on the other changes I will post these to the valgrind-developers list first. I'm not so impolite to roll back changes without prior discussion. Bart. |