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

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

[Petr Vorel]

Previously with --disable-metadata output didn't mention that metadata
are disabled and printed config which was not used. Now:

$ ./configure --disable-metadata
...
METADATA
metadata disabled

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

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 configure.ac | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/configure.ac b/configure.ac
index 3c56d19224..5b9e3c1781 100644
--- a/configure.ac
+++ b/configure.ac
@@ -404,7 +404,14 @@ libtirpc: ${have_libtirpc:-no}
 glibc SUN-RPC: ${have_rpc_glibc:-no}
 
 METADATA
+EOF
+
+if test "x$enable_metadata" = xyes; then
+cat << EOF
 metadata generator: $with_metadata_generator
 HTML metadata: $with_metadata_html
 PDF metadata: $with_metadata_pdf
 EOF
+else
+echo metadata disabled
+fi
-- 
2.34.1


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

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

[Joerg Vehlow]

Hi Petr,


Am 1/14/2022 um 1:55 PM schrieb Petr Vorel:
> Previously with --disable-metadata output didn't mention that metadata
> are disabled and printed config which was not used. Now>
> $ ./configure --disable-metadata

a bit out of context here, but still: The disable-metadata flag is
confusing, because it does not disable metadata entirely. We had to
patch the makefile, to disable build of mateparse. While this is
basically setup correctly for cross compilation, we had some problems,
if the HOSTCC version did not match the CC version.
I am not entirely sure what the problem was, only that we did not care,
because we don't need metaparse end disabling it was simpler than fixing
the build.

Long story short: "--disable-metadata" should completely disable
metadata parsing, not only document generation

Joerg

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

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

[Cyril Hrubis]

Hi!
> Previously with --disable-metadata output didn't mention that metadata
> are disabled and printed config which was not used. Now:
>
> $ ./configure --disable-metadata

Slightly off topic, should we rename this to --disable-docparse or
--disable-autodoc?

Since we split the metadata generator from the documentation and the
metadata are now genereated unconditionally...

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Joerg Vehlow]

Hi

Am 1/14/2022 um 3:12 PM schrieb Cyril Hrubis:
> Hi!
>> Previously with --disable-metadata output didn't mention that metadata
>> are disabled and printed config which was not used. Now:
>>
>> $ ./configure --disable-metadata
> 
> Slightly off topic, should we rename this to --disable-docparse or
> --disable-autodoc?
> 
> Since we split the metadata generator from the documentation and the
> metadata are now genereated unconditionally...
> 

I would love that and I would reintroduce disable-metadata in that case,
to completely disable it.
The question is: Will this break some builds because of semantic changes?

Joerg

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

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

[Cyril Hrubis]

Hi!
> >> Previously with --disable-metadata output didn't mention that metadata
> >> are disabled and printed config which was not used. Now:
> >>
> >> $ ./configure --disable-metadata
> > 
> > Slightly off topic, should we rename this to --disable-docparse or
> > --disable-autodoc?
> > 
> > Since we split the metadata generator from the documentation and the
> > metadata are now genereated unconditionally...
> > 
> 
> I would love that and I would reintroduce disable-metadata in that case,
> to completely disable it.
> The question is: Will this break some builds because of semantic changes?

The JSON metadata file is going to replace the runtest files some day,
at least that is the longterm plan. Also the parser that extracts the
metadata from the sources is rather fast, compared to the LTP build and
it's self contained. There is really no reason to have a switch to
disable the metadat extraction step.

The documentation build, on the other hand, is rather slow and requires
asciidoc, which is the reason why there is a switch that can be used to
disable that. The only problem here is that the name is a bit confusing
now.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Joerg Vehlow]

Hi

Am 1/14/2022 um 3:23 PM schrieb Cyril Hrubis:
> The JSON metadata file is going to replace the runtest files some day,
> at least that is the longterm plan. Also the parser that extracts the
> metadata from the sources is rather fast, compared to the LTP build and
> it's self contained. There is really no reason to have a switch to
> disable the metadat extraction step.
> 
> The documentation build, on the other hand, is rather slow and requires
> asciidoc, which is the reason why there is a switch that can be used to
> disable that. The only problem here is that the name is a bit confusing
> now.
> 
Actually there is, that is where my interest in this flag comes from. We
don't use runtest files (at least not for execution), so we don't need
it and we had problems in out cross build environment, that were not
easily fixed. From looking at the source I am not sure what the problem
was and cannot spot it from looking at the source. But I know, that it
wasn't just fixed by setting the correct HOSTCC and HOST_CFLAGS. I have
a missing library in my head, but I can't see any.
All I know is that I gave up fixing the build and I don't give up easily
normally (a little easier if the functionality is not needed at all) :)

Joerg

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

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

[Cyril Hrubis]

Hi!
> > The JSON metadata file is going to replace the runtest files some day,
> > at least that is the longterm plan. Also the parser that extracts the
> > metadata from the sources is rather fast, compared to the LTP build and
> > it's self contained. There is really no reason to have a switch to
> > disable the metadat extraction step.
> > 
> > The documentation build, on the other hand, is rather slow and requires
> > asciidoc, which is the reason why there is a switch that can be used to
> > disable that. The only problem here is that the name is a bit confusing
> > now.
> > 
> Actually there is, that is where my interest in this flag comes from. We
> don't use runtest files (at least not for execution), so we don't need
> it and we had problems in out cross build environment, that were not
> easily fixed. From looking at the source I am not sure what the problem
> was and cannot spot it from looking at the source. But I know, that it
> wasn't just fixed by setting the correct HOSTCC and HOST_CFLAGS. I have
> a missing library in my head, but I can't see any.

That is really strange, there are practically no dependencies for the
metadata extractor. All that you have to have is a C compiler and libc.

> All I know is that I gave up fixing the build and I don't give up easily
> normally (a little easier if the functionality is not needed at all) :)

You should have reported that on the list. It's going to be really
cornerstone of the way how tests are executed so the build should get
fixed anyways.

I guess that we can add a flag that disables the metadata extraction
step, with a big red warning that describes the consequencies of the
choice.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

> Hi!
> > > The JSON metadata file is going to replace the runtest files some day,
> > > at least that is the longterm plan. Also the parser that extracts the
> > > metadata from the sources is rather fast, compared to the LTP build and
> > > it's self contained. There is really no reason to have a switch to
> > > disable the metadat extraction step.

> > > The documentation build, on the other hand, is rather slow and requires
> > > asciidoc, which is the reason why there is a switch that can be used to
> > > disable that. The only problem here is that the name is a bit confusing
> > > now.

> > Actually there is, that is where my interest in this flag comes from. We
> > don't use runtest files (at least not for execution), so we don't need
> > it and we had problems in out cross build environment, that were not
> > easily fixed. From looking at the source I am not sure what the problem
> > was and cannot spot it from looking at the source. But I know, that it
> > wasn't just fixed by setting the correct HOSTCC and HOST_CFLAGS. I have
> > a missing library in my head, but I can't see any.

> That is really strange, there are practically no dependencies for the
> metadata extractor. All that you have to have is a C compiler and libc.

> > All I know is that I gave up fixing the build and I don't give up easily
> > normally (a little easier if the functionality is not needed at all) :)

> You should have reported that on the list. It's going to be really
> cornerstone of the way how tests are executed so the build should get
> fixed anyways.
It's a question if it's a problem of building C or a problem of integrating LTP
build to another build system.

> I guess that we can add a flag that disables the metadata extraction
> step, with a big red warning that describes the consequencies of the
> choice.
Let's see if we can solve the problem. If not, it might help also to others to
have flag disabling all metadata build (probably named --disable-metadata
and name current --disable-metadata functionality as --disable-docparse).

Kind regards,
Petr

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

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

[Petr Vorel]

Hi Joerg,

> Hi

> Am 1/14/2022 um 3:23 PM schrieb Cyril Hrubis:
> > The JSON metadata file is going to replace the runtest files some day,
> > at least that is the longterm plan. Also the parser that extracts the
> > metadata from the sources is rather fast, compared to the LTP build and
> > it's self contained. There is really no reason to have a switch to
> > disable the metadat extraction step.

> > The documentation build, on the other hand, is rather slow and requires
> > asciidoc, which is the reason why there is a switch that can be used to
> > disable that. The only problem here is that the name is a bit confusing
> > now.
Cyril, maybe we should have kept the way to completely disable building metadata
until we actually use it (i.e. until runtest files are replaced).

OTOH this way give the change to (embedded) distros to fix host build setup
issues :). I was testing Buildroot last night and it has problems with build on
archs which have cross-compile flags (CFLAGS) incompatible with host flags
(HOST_CFLAGS). This might require deeper changes to introduce LTP host build
(Buildroot support host autotools builds, but LTP is not fully autootools
project - we use only autoconf).

> Actually there is, that is where my interest in this flag comes from. We
> don't use runtest files (at least not for execution), so we don't need
> it and we had problems in out cross build environment, that were not
> easily fixed. From looking at the source I am not sure what the problem
> was and cannot spot it from looking at the source. But I know, that it
> wasn't just fixed by setting the correct HOSTCC and HOST_CFLAGS. I have
> a missing library in my head, but I can't see any.
Hard to help without seeing logs.

Kind regards,
Petr

> All I know is that I gave up fixing the build and I don't give up easily
> normally (a little easier if the functionality is not needed at all) :)

> Joerg

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

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

[Cyril Hrubis]

Hi!
> Previously with --disable-metadata output didn't mention that metadata
> are disabled and printed config which was not used. Now:
> 
> $ ./configure --disable-metadata
> ...
> METADATA
> metadata disabled
> 
> $ ./configure
> ...
> METADATA
> metadata generator: asciidoctor
> HTML metadata: yes
> PDF metadata: no
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  configure.ac | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 3c56d19224..5b9e3c1781 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -404,7 +404,14 @@ libtirpc: ${have_libtirpc:-no}
>  glibc SUN-RPC: ${have_rpc_glibc:-no}
>  
>  METADATA
> +EOF
> +
> +if test "x$enable_metadata" = xyes; then
> +cat << EOF
>  metadata generator: $with_metadata_generator
>  HTML metadata: $with_metadata_html
>  PDF metadata: $with_metadata_pdf

Don't we stil have the same problem with "$enable_metadata_html" and
"$enable_metadata_pdf" ?

Also looking at m4/ltp-docparse.m4 shouldn't we just skip the
autodetection if metadata are disabled and exit with all three wariables
set to no?

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.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

Hi Cyril,

> Hi!
> > Previously with --disable-metadata output didn't mention that metadata
> > are disabled and printed config which was not used. Now:

> > $ ./configure --disable-metadata
> > ...
> > METADATA
> > metadata disabled

> > $ ./configure
> > ...
> > METADATA
> > metadata generator: asciidoctor
> > HTML metadata: yes
> > PDF metadata: no

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  configure.ac | 7 +++++++
> >  1 file changed, 7 insertions(+)

> > diff --git a/configure.ac b/configure.ac
> > index 3c56d19224..5b9e3c1781 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -404,7 +404,14 @@ libtirpc: ${have_libtirpc:-no}
> >  glibc SUN-RPC: ${have_rpc_glibc:-no}

> >  METADATA
> > +EOF
> > +
> > +if test "x$enable_metadata" = xyes; then
> > +cat << EOF
> >  metadata generator: $with_metadata_generator
> >  HTML metadata: $with_metadata_html
> >  PDF metadata: $with_metadata_pdf

> 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.

> Also looking at m4/ltp-docparse.m4 shouldn't we just skip the
> autodetection if metadata are disabled and exit with all three wariables
> set to no?
Well, but that's how it is implemented:
$ ./configure --disable-metadata
...
configure: metadata generation disabled

And variables in config.log are:
METADATA_GENERATOR=''
WITH_METADATA='no'
WITH_METADATA_HTML='no'
WITH_METADATA_PDF='no'

It's a bit hard to write m4 macros when there is no return :).
It's hard to not fallback into many if/else levels.
I should have put some comments into the code.

Also, currently it's implemented that that we can disable both HTML and
PDF as: --disable-metadata-html --disable-metadata-pdf, maybe we could rethink
it.

> 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 wonder what is needed to be fixed now and what's better to postpone after the
release?

Kind regards,
Petr

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

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

[Cyril Hrubis]

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

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.

> > 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.

> 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.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

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