| Message ID | 20220114125513.895-1-pvorel@suse.cz |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [1/1] configure.ac: Fix summary for disabled metadata | expand |
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
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...
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
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.
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
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.
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
> 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
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.
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
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.
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
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
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(+)