Re: [LTP] [PATCH 1/1] configure.ac: Fix summary for disabled metadata

[Petr Vorel <pvorel@suse.cz>]

Hi Cyril,

> Hi!
> > > Don't we stil have the same problem with "$enable_metadata_html" and
> > > "$enable_metadata_pdf" ?
> > Well, this patch was about configure output. I wouldn't print them at all if
> > metadata disabled. They're not used anyway in the code if metadata disabled.

> I guess that I'm still confused by the configure script, because we do
> use enable_metadata_html and enable_metadata_pdf but I do not get how
> the enable_metadata works, we do not seem to use it for anything but
> perl module check in there.

> What I would have expected there would be:

> diff --git a/m4/ltp-docparse.m4 b/m4/ltp-docparse.m4
> index 88d2e08e4..6f0bef1c9 100644
> --- a/m4/ltp-docparse.m4
> +++ b/m4/ltp-docparse.m4
> @@ -35,7 +35,12 @@ with_metadata=no
>  with_metadata_html=no
>  with_metadata_pdf=no

> -if test "x$enable_metadata" = xyes && test "x$enable_metadata_html" = xyes -o "x$enable_metadata_pdf" = xyes; then
> +if test "x$enable_metadata" != xyes; then
> +       enable_metadata_html=no
> +       enable_metadata_pdf=no
> +fi
> +
> +if test "x$enable_metadata_html" = xyes -o "x$enable_metadata_pdf" = xyes; then
>         AX_PROG_PERL_MODULES(Cwd File::Basename JSON LWP::Simple)
>  fi

+1 to this code. Thanks!

> And that would cause both with_metadata_html and with_metadata_pdf to be
> set to 'no' and the configure summary would be correct to begin with.

$ ./configure --disable-metadata
METADATA
metadata generator: detect
HTML metadata: no
PDF metadata: no

$ ./configure --disable-metadata-pdf --disable-metadata-html
METADATA
metadata generator: detect
HTML metadata: no
PDF metadata: no

$ ./configure --disable-metadata-html
METADATA
metadata generator: detect
HTML metadata: no
PDF metadata: no

$ ./configure --disable-metadata-pdf
METADATA
metadata generator: asciidoctor
HTML metadata: yes
PDF metadata: no

Well, everything works as expected (not a surprise), but not sure if "detect" is
not confusing people.  I's still prefer just saying there are no metadata generated.
But that's the presentation in configure summary. Also no strong feeling about it :).
You can merge this patch with my ack, that's an improvement anyway.

> > > I think that we should rethink what the flags really do, I guess that
> > > for instance it would make sense for the $enable_metadata=no to just set
> > > both $enable_metadata_html and $enable_metadata_pdf to no and the rest
> > > of the m4/ltp-docparse.m4 should just check the later two.
> > Rethink the flags meaning or a functionality?

> > And how about flags names? What name would you suggest?
> > --disable-doc, --with-html-doc, --with-pdf-doc ?
> > Because we now have metadata (i.e. JSON output) mandatory.

> I guess that I would call this 'autodoc' or 'docparse' or something
> that describes that it's generated documentation.
Again, isn't it "docparse" or "autodoc" confusing for users?

I'd prefer to have just "doc", but agree, doc are documentation files in doc/,
which we don't transform to html/pdf (we could in the future, but that's a
different story).

I mean if we change flag, which requires user to notice and distros update
packaging recipes it should be really an improvement. Maybe
--metadata-doc{,-html,-pdf} ?

> > I wonder what is needed to be fixed now and what's better to postpone after the
> > release?

> Depends on the size of the patch, if it's small enough, like the one I
> posted above it should be okay.

Patch like this is OK. Also simple rename of flags would work. I thought you
wanted a bigger rewrite.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp