Re: [gpsd-dev] [PATCH 4/4] Parallelizes gps-regress and gps-makeregress.

[Fred Wright]

On Mon, 29 Feb 2016, Robert Norris wrote:

> > Date: Mon, 29 Feb 2016 14:13:31 -0800
> > From: address@hidden
[...]
> >
> > Thanks, though note that one of my earlier as yet uncommitted patches is
> > needed to make this maximally useful. Running 'check' in parallel doesn't
> > work due to the implied 'testclean' stomping on other tests while they're
> > running, hence the new 'test-noclean'. Though running 'gps-regress' in
> > parallel by itself works fine without the other patch.
>
> I think it's better to simply remove the use of testclean within the
> check stage as it seems pointless (remove the testclean completely� and

I personally agree, but since the history indicates that the self-cleaning
test was considered desirable when it was implemented, I didn't want to
change that.  Changing the meaning of 'check' might be controversial, but
adding a new target that omits the 'testclean' isn't.

> also 'test_spec' within the check stage as the time_regress will
> automatically build it if necessary).

I presume you mean 'test_timespec', which indeed appears to be redundant
in this context.

> I don't understand why these where added and anyway the clean stage
> removes all generated output (i.e. including the test programs).

There are numerous things that could be cleaned up (so to speak) with
regard to cleaning, though there's no way to get scons's automatic
cleaning support to provide a way to clean only the test programs.  In
'clean' mode, it cleans all dependencies of the specified target(s), even
ones that are needed by targets not being cleaned.

The default clean doesn't remove test programs, because they're not part
of the default build.  But a better way to handle that is to make the
default target different in 'clean' mode.

> Instead I was going to post this simple patch:
>
> diff --git a/SConstruct b/SConstruct
> index ce64983..8058925 100644
> @@ -1757,6 +1757,7 @@ testclean = Utility('testclean', [],
> �check = env.Alias('check', [
> ���� describe,
> ���� python_compilation_regress,
> +��� testprogs,
> ���� method_regress,
> ���� bits_regress,
> ���� matrix_regress,

A good illustration of why pasting patches directly into the HotMail body
is a bad idea if done "for real". :-) Though HotMail's insistence on
treating all attachments as binary is also inconvenient.

> Thus this ensures *all* test programs are at least compiled (and should have 
> flagged the failing test_gpsmm earlier).

The purpose of 'check' is to run a particular subset of the tests.
Verifying that tests that it's *not* running build properly is beyond its
job description.  Every test that it *does* run depends on the programs it
needs, so adding 'testprogs' to 'check' is completely redundant.  If you
want to make sure that everything builds, build everything. :-) Perhaps a
'buildall' target would be useful, but "scons build testprogs" works now.

Probably the *real* issue in this case is that 'check' doesn't include
*running* test_gpsmm, but that would be nontrivial since it requires a
running daemon instance.

> Not all test programs are actually automatically run as part of tests,
> �(which only builds the needed programs) thus ensuring they all compile.

Exactly.  See above.

BTW, there are a few things wrong with your test_gpsmm dbus fix:

1) Currently, the way dbus linking is normally handled is via the extra
options added to LIBPATH.  The only reason test_gpsmm didn't build is due
to the LIBPATH override that I (and at least one other person) have
previously complained about.  If it weren't for that bug, your fix would
be unnecessary.

2) Although one might argue that adding dbusflags to the specific places
where it's needed is better than adding the optons globally to LIBPATH,
the existing definition of dbusflags isn't actually valid for that,
because it only provides the '-ldbus-1', not the complete set of options
(including the relevant library path) to properly link the dbus library.
Your fix only works on plaforms where the dbus library is installed in the
"system" library area, which seems to be limited to Linux.  The LIBPATH
setup, OTOH, includes the complete set of options obtained from
pkg-config, and works on all platforms.

3) Regardless of which approach is taken to providing the dbus link
options, it should be done consistently, rather than being done one way
for one program and the other way for everything else.  And if the
(corrected) dbusflags approach is used, it shouldn't *also* add the dbus
flags to LIBPATH with MergeFlags().

> Presumably this would also be beneficial for running regression tests in 
> parallel.

I don't see how it has anything to do with that.  The only issues with
parallelism were:

1) Running without 'testclean', since there seems to be no way to get it
to sequence properly without creating undesirable dependencies (e.g.,
"Requires()" doesn't work as desired).

2) Reworking the daemon tests to run as separate single-case tests.

On Mon, 29 Feb 2016, Jon Schlueter wrote:

> thanks for catching, that.
>
> All merged

This is an example of where making better use of GitHub (where available)
might be less error-prone.

When I originally forked your GitHub repo to make mine, the workflow I
envisioned involved using GitHub pull requests to merge into your repo,
which you could then push to Savannah.  It hasn't worked out that way,
after Eric complained about having issues converting a GitHub pull request
to actual patches.  I don't know if there is a reasonable way to derive
patches from a GitHub pull request directly, but it's certainly
straightforward to fetch a GitHub branch to a local repo (regardless of
whether the fetcher has a GitHub account):

git fetch <GitHub repo URL> +<GitHub branch>:<local branch>

The '+' isn't needed initially, but it doesn't hurt and *is* needed when
updating the local branch from a rebased GitHub branch.

Since this is only a fetch, it doesn't do any merging and doesn't touch
the index or working directory, and is very fast when fetching something
that's only a few commits away from the local repo state.  The only
potential for conflict is in the local branch name, which might be
"namespaced" by prepending the GitHub userid to the local branch name.:

git fetch <GitHub repo URL> +<GitHub branch>:<GitHub userid>-<local branch>

Once the commits are in the local repo, all the local git tools are
available to view, diff, merge, etc.

With regard to the "missed patches" problem referenced above, if only a
subset of the commits are merged, then the rebased branch will still
contain the unmerged commits, making it less likely that they'll fall
through a crack.  Git would provide patch tracking automatically.

I'd also expect "git fetch" to be more robust than sending patches through
email.

One objection might be that this approach doesn't make the descriptions
and diffs directly viewable in the email (though that's already true in
cases where the patches are "binary" attachments, at least with some email
clients).  But that could be addressed by including a --oneline log in the
email text, as well as the URL for directly viewing the diffs on the
GitHub website:

        <GitHub repo URL>/compare/master...<GitHub branch>

Fred Wright