On Fri, 2013-03-22 at 19:43 +0100, David Henningsson wrote:
> On 03/22/2013 07:00 PM, Tanu Kaskinen wrote:
> > On Fri, 2013-03-22 at 15:37 +0100, David Henningsson wrote:
> >> It checks all files in the mixer/paths directory and checks
> >> - that the file can be parsed without errors
> >> - that the file is actually shipped in the makefile
> >>
> >> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> >> ---
> >>
> >> After I realized that my previous headset-mic patch did not event ship the new file,
> >> I also realized I might do the same mistake again.
> >>
> >> I guess this could be enhanced later with checking that profile-sets do not point
> >> to non-existent paths etc, but this will do for now.
> >>
> >> src/Makefile.am | 7 +++
> >> src/tests/alsa-mixer-path-test.c | 110 ++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 117 insertions(+)
> >> create mode 100644 src/tests/alsa-mixer-path-test.c
> >>
> >> diff --git a/src/Makefile.am b/src/Makefile.am
> >> index 7685e0c..3326a2c 100644
> >> --- a/src/Makefile.am
> >> +++ b/src/Makefile.am
> >> @@ -297,6 +297,8 @@ endif
> >> if HAVE_ALSA
> >> TESTS_norun += \
> >> alsa-time-test
> >> +TESTS_default += \
> >> + alsa-mixer-path-test
> >> endif
> >>
> >> if HAVE_TESTS
> >> @@ -545,6 +547,11 @@ alsa_time_test_LDADD = $(AM_LDADD) $(ASOUNDLIB_LIBS)
> >> alsa_time_test_CFLAGS = $(AM_CFLAGS) $(ASOUNDLIB_CFLAGS)
> >> alsa_time_test_LDFLAGS = $(AM_LDFLAGS) $(BINLDFLAGS)
> >>
> >> +alsa_mixer_path_test_SOURCES = tests/alsa-mixer-path-test.c
> >> +alsa_mixer_path_test_CFLAGS = $(AM_CFLAGS) $(LIBCHECK_CFLAGS) $(ASOUNDLIB_CFLAGS)
> >> +alsa_mixer_path_test_LDADD = $(AM_LDADD) libpulsecore- at PA_MAJORMINOR@.la libpulse.la libpulsecommon- at PA_MAJORMINOR@.la libalsa-util.la
> >> +alsa_mixer_path_test_LDFLAGS = $(AM_LDFLAGS) $(BINLDFLAGS) $(LIBCHECK_LIBS)
> >> +
> >> usergroup_test_SOURCES = tests/usergroup-test.c
> >> usergroup_test_LDADD = $(AM_LDADD) libpulsecore- at PA_MAJORMINOR@.la libpulse.la libpulsecommon- at PA_MAJORMINOR@.la
> >> usergroup_test_CFLAGS = $(AM_CFLAGS) $(LIBCHECK_CFLAGS)
> >> diff --git a/src/tests/alsa-mixer-path-test.c b/src/tests/alsa-mixer-path-test.c
> >> new file mode 100644
> >> index 0000000..2460c66
> >> --- /dev/null
> >> +++ b/src/tests/alsa-mixer-path-test.c
> >> @@ -0,0 +1,110 @@
> >> +#ifdef HAVE_CONFIG_H
> >> +#include <config.h>
> >> +#endif
> >> +
> >> +#include <check.h>
> >> +#include <dirent.h>
> >> +#include <stdbool.h>
> >> +#include <stdio.h>
> >> +
> >> +#include <pulse/pulseaudio.h>
> >> +#include <pulsecore/log.h>
> >> +#include <pulsecore/core-util.h>
> >> +#include <pulsecore/strlist.h>
> >> +#include <modules/alsa/alsa-mixer.h>
> >> +
> >> +
> >> +static const char *get_default_paths_dir(void) {
> >> + if (pa_run_from_build_tree())
> >> + return PA_BUILDDIR "/modules/alsa/mixer/paths/";
> >
> > This doesn't work if the build directory is different from the source
> > directory. Instead of PA_BUILDDIR, this should use PA_SRCDIR, which
> > doesn't currently exist, but it should be possible to add it.
>
> It is copy-pasted from existing pulseaudio code (in alsa-mixer.c, IIRC).
> If you fix it there, feel free to do so here to.
OK.
> But I do an out-of-source build normally (according to Colin's old but
> excellent instructions), and my test worked siccessfully. I do remember
> though that Colin's instructions include making some related symlink.
I tried this:
mkdir build
cd build
../configure
make check
This test failed.
> >
> >> + else
> >> + return PA_ALSA_PATHS_DIR;
> >> +}
> >> +
> >> +static pa_strlist *load_makefile() {
> >> + FILE *f;
> >> + bool lookforfiles = false;
> >> + char buf[2048];
> >> + pa_strlist *result = NULL;
> >> + const char *Makefile = PA_BUILDDIR "/Makefile";
> >> +
> >> + f = pa_fopen_cloexec(Makefile, "r");
> >> + fail_unless(f != NULL); /* Consider skipping this test instead of failing if Makefile not found? */
> >
> > If you want my opinion to the question, I think it's fine to assume that
> > Makefile is always found.
>
> Ok.
>
> >
> >> + while (!feof(f)) {
> >> + if (!fgets(buf, sizeof(buf), f)) {
> >> + fail_unless(feof(f));
> >
> > The loop condition is !feof(f), so this will always fail. The if could
> > be replaced with fail_unless(fgets(buf, sizeof(buf), f));
>
> Eh, I don't think so?
fgets() returns NULL in case there's an error or f is at EOF. The while
condition just checked that f is not at EOF, therefore an error must
have happened.
> If you're sure, could you please correct the code in pa_config_parse
> which works the same way?
Done.
--
Tanu
