Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build documentation with doxygen-clang #3744

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aPonza
Copy link
Contributor

@aPonza aPonza commented Mar 13, 2020

This is the result of the discussion had in #3712 in order to continue the work in #3701.

I'm sure I'm missing the latex dependencies at the moment, but I would like to understand your thought process to having dvipng in the previous version instead of the strictly required packages. From some scouting (here and looking at the previous build) I know I miss a subset of:

  • cm-super-minimal
  • ghostscript;
  • graphviz;
  • texlive-extra-utils;
  • texlive-font-utils;
  • texlive-fonts-recommended;
  • texlive-latex-extra;
  • texlive-latex-recommended.

I also want to confirm that all the warnings/errors seen aren't due to real things clang has a reason to flag. Like e.g. the first batch of warnings are from the namespace issue from PCL's rule that in implementation files names must be pcl::module_name::ClassName::function instead of using the namespace keyword.

@aPonza aPonza force-pushed the use_clang_parser branch 2 times, most recently from 2e9bd36 to 61795d4 Compare March 13, 2020 10:23
@kunaltyagi
Copy link
Member

kunaltyagi commented Mar 13, 2020

Yeah... If the recursion warnings don't go away, clang + doxygen isn't worth it. Since you are building from source, could you try once with the latest release?

On a separate note, unless the clang errors go away, it doesn't seem worth it to use clang-doxygen integration, even if the recursion warnings go away.

Originally posted by @kunaltyagi in #3712 (comment)

Did you get improved results in parsing? ...holding breath here...

@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation module: ci needs: author reply Specify why not closed/merged yet skill: doxygen Skills/areas of expertise needed to tackle the issue labels Mar 13, 2020
@aPonza
Copy link
Contributor Author

aPonza commented Mar 13, 2020

I'm trying locally with 1.8.17. I'm keen on thinking that what we're seeing with the clang parsing is due to real problems in the code, and want to test it, as stated in the OP.

If you have time, could you give me a hint on how to make the documentation job work? It doesn't like /usr/bin/clang so I changed it to ${which clang} which it also doesn't like. The problem is in the yaml, currently.

Would it be a bad idea trying to cross-post on the doxygen tracker after the pipeline works? I would like to ask about the std:: errors which shouldn't happen with the compilation database and about the general flags passed to clang as every warning is outputted as "error" even though compilation runs anyways. To do this I would need a shared build log though.

@kunaltyagi
Copy link
Member

could you give me a hint on how to make the documentation job work

Most likely because the modifications in dockerfile don't mean that the image actually changed (there's no job to automatically push the modified docker file)

Would it be a bad idea trying to cross-post on the doxygen tracker after the pipeline works?

Definitely not. However, it might help more if you could create a docker-file individually which you pushed so others can pull it easily. The pointcloudlibrary doc will require a success-story to change (no need to manually compile otherwise)

@aPonza
Copy link
Contributor Author

aPonza commented Mar 13, 2020

The pointcloudlibrary doc will require a success-story to change

You'd be right in saying I edit too much my comments, but it did succeed #3712 (comment) and that's the main reason I think it should eventually work again without all the header renames. I thought the compilation database would be enough but that's not doing it somehow.

I'm pushing the image I have locally, didn't know you preferred this way instead of pulling the Dockerfile and building. It seemed faster to me this other way.

@aPonza aPonza force-pushed the use_clang_parser branch from 61795d4 to 34f634c Compare March 13, 2020 14:06
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing, you may push the built Docker image to your private Dockerhub space and modify line 4 in the 'documentation.yml' file to point to your image.

.ci/azure-pipelines/documentation.yml Outdated Show resolved Hide resolved
@aPonza aPonza force-pushed the use_clang_parser branch 2 times, most recently from adc8317 to dd03456 Compare March 13, 2020 23:07
@aPonza
Copy link
Contributor Author

aPonza commented Mar 14, 2020

It works locally, I don't understand. I run the same exact commands. just realized the CI pipeline needs a cd every time, which is missing in the second configuration step.

Excerpt of the build I ran tonight
[...]

/pcl/visualization/include/pcl/visualization/pcl_visualizer.h:125: warning: argument 'custom' of command @param is not found in the argument list of pcl::visualization::PCLVisualizer::PCLVisualizer(int &argc, char **argv, vtkSmartPointer< vtkRenderer > ren, vtkSmartPointer< vtkRenderWindow > wind, const std::string &name="", PCLVisualizerInteractorStyle *style=PCLVisualizerInteractorStyle::New(), const bool create_interactor=true)
/pcl/visualization/include/pcl/visualization/pcl_visualizer.h:132: warning: The following parameters of pcl::visualization::PCLVisualizer::PCLVisualizer(int &argc, char **argv, vtkSmartPointer< vtkRenderer > ren, vtkSmartPointer< vtkRenderWindow > wind, const std::string &name="", PCLVisualizerInteractorStyle *style=PCLVisualizerInteractorStyle::New(), const bool create_interactor=true) are not documented:
  parameter 'ren'
  parameter 'wind'
  parameter 'name'
/pcl/visualization/include/pcl/visualization/point_cloud_color_handlers.h:526: warning: The following parameters of pcl::visualization::PointCloudColorHandlerLabelField::PointCloudColorHandlerLabelField(const PointCloudConstPtr &cloud, const bool static_mapping=true) are not documented:
  parameter 'cloud'
/pcl/visualization/include/pcl/visualization/point_cloud_color_handlers.h:913: warning: The following parameters of pcl::visualization::PointCloudColorHandlerLabelField< pcl::PCLPointCloud2 >::PointCloudColorHandlerLabelField(const PointCloudConstPtr &cloud, const bool static_mapping=true) are not documented:
  parameter 'cloud'
Built target doc
Scanning dependencies of target tutorials
Running Sphinx v2.4.4
WARNING: while setting up extension sphinxcontrib.doxylink.doxylink: extension 'sphinxcontrib.doxylink.doxylink' has no setup() function; is it really a Sphinx extension module?
making output directory... done
building [mo]: all of 0 po files
building [html]: all source files
updating environment: [new config] 98 added, 0 changed, 0 removed
reading sources... [100%] writing_pcd                                                                                                                                                                       
/pcl/doc/tutorials/content/alignment_prerejective.rst:35: WARNING: Unknown interpreted text role "pcl".
/pcl/doc/tutorials/content/alignment_prerejective.rst:41: WARNING: Unknown interpreted text role "pcl".
/pcl/doc/tutorials/content/alignment_prerejective.rst:47: WARNING: Unknown interpreted text role "pcl".

[...]

/pcl/doc/tutorials/content/writing_new_classes.rst: WARNING: document isn't included in any toctree
/pcl/doc/tutorials/content/writing_pcd.rst: WARNING: document isn't included in any toctree
done
preparing documents... done
writing output... [100%] writing_pcd                                                                                                                                                                        
WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting
/pcl/doc/tutorials/content/davidsdk.rst:30: WARNING: Could not lex literal_block as "cmake". Highlighting skipped.
/pcl/doc/tutorials/content/global_hypothesis_verification.rst:111: WARNING: Could not lex literal_block as "c++". Highlighting skipped.
/pcl/doc/tutorials/content/global_hypothesis_verification.rst:118: WARNING: Could not lex literal_block as "c++". Highlighting skipped.
/pcl/doc/tutorials/content/global_hypothesis_verification.rst:125: WARNING: Could not lex literal_block as "c++". Highlighting skipped.
/pcl/doc/tutorials/content/global_hypothesis_verification.rst:143: WARNING: Could not lex literal_block as "c++". Highlighting skipped.
/pcl/doc/tutorials/content/using_pcl_pcl_config.rst:205: WARNING: Could not lex literal_block as "cmake". Highlighting skipped.
/pcl/doc/tutorials/content/writing_new_classes.rst:254: WARNING: Could not lex literal_block as "cmake". Highlighting skipped.
generating indices... done
writing additional pages...  searchdone
copying images... [100%] images/octree_bunny2.png                                                                                                                                                           
copying downloadable files... [100%] ../../build/doc/tutorials/sources/template_alignment/template_alignment.cpp                                                                                            
copying static files... ... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 195 warnings.

The HTML pages are in html.
Built target tutorials
error: /pcl/2d/include/pcl/2d/impl/convolution.hpp:44:1: error: use of undeclared identifier 'pcl' [clang]
error: /pcl/2d/include/pcl/2d/impl/edge.hpp:47:6: error: no member named 'Edge' in namespace 'pcl' [clang]
error: /pcl/2d/include/pcl/2d/impl/edge.hpp:47:10: error: expected ';' at end of declaration [clang]
error: /pcl/2d/include/pcl/2d/impl/edge.hpp:47:10: error: expected unqualified-id [clang]
error: /pcl/2d/include/pcl/2d/impl/edge.hpp:85:31: error: qualified name refers into a specialization of variable template 'pcl::Edge' [clang]

[...]

/pcl/visualization/include/pcl/visualization/point_cloud_color_handlers.h:526: warning: The following parameters of pcl::visualization::PointCloudColorHandlerLabelField::PointCloudColorHandlerLabelField(const PointCloudConstPtr &cloud, const bool static_mapping=true) are not documented:
  parameter 'cloud'
/pcl/visualization/include/pcl/visualization/point_cloud_color_handlers.h:913: warning: The following parameters of pcl::visualization::PointCloudColorHandlerLabelField< pcl::PCLPointCloud2 >::PointCloudColorHandlerLabelField(const PointCloudConstPtr &cloud, const bool static_mapping=true) are not documented:
  parameter 'cloud'
Built target doc
Scanning dependencies of target advanced
Running Sphinx v2.4.4
WARNING: while setting up extension sphinxcontrib.doxylink.doxylink: extension 'sphinxcontrib.doxylink.doxylink' has no setup() function; is it really a Sphinx extension module?
making output directory... done
WARNING: The config value `html_title' has type `NoneType'; expected `str'.
WARNING: The config value `html_add_permalinks' has type `NoneType', defaults to `str'.
loading pickled environment... failed
failed: source directory has changed
building [mo]: all of 0 po files
building [html]: all source files
updating environment: [new config] 14 added, 0 changed, 0 removed
reading sources... [100%] vertical_sse                                                                                                                                                                      
/pcl/doc/advanced/content/exceptions_guide.rst:13: WARNING: Unknown interpreted text role "pcl".
/pcl/doc/advanced/content/index.rst:100: WARNING: Title underline too short.

Committing changes to the git master
-----------------------------------
/pcl/doc/advanced/content/index.rst:100: WARNING: Title underline too short.

[...]

copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 40 warnings.

The HTML pages are in html.
Built target advanced

With this history:

   34  clear
   35  cmake -DCMAKE_C_COMPILER=$(which clang) -DCMAKE_CXX_COMPILER=$(which clang++) -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_CUDA=ON -DBUILD_GPU=ON -DBUILD_apps=ON -DBUILD_apps_3d_rec_framework=ON -DBUILD_apps_cloud_composer=ON -DBUILD_apps_in_hand_scanner=ON -DBUILD_apps_modeler=ON -DBUILD_apps_point_cloud_editor=ON -DBUILD_examples=ON -DBUILD_global_tests=ON -DBUILD_outofcore=ON -DBUILD_people=ON -DBUILD_simulation=ON -DBUILD_surface_on_nurbs=ON -DBUILD_visualization=ON ../
   36  cmake -DDOXYGEN_USE_SHORT_NAMES=OFF -DSPHINX_HTML_FILE_SUFFIX=php -DWITH_DOCS=ON -DWITH_TUTORIALS=ON ../
   37  cmake --build . --parallel $(nproc) -- doc tutorials advanced
   38  history

@aPonza
Copy link
Contributor Author

aPonza commented Mar 14, 2020

Also I was going crazy trying to link libc++/libc++abi/libstdc++/... but, these errors:

error: /pcl/common/include/pcl/common/bivariate_polynomial.h:93:32: error: no template named 'vector' in namespace 'std' [clang]

make absolute sense, since these are the only includes

#pragma once
#include <fstream>
#include <iostream>

when clearly #include <vector> is absolutely needed

findCriticalPoints (std::vector<real>& x_values, std::vector<real>& y_values, std::vector<int>& types) const;

@aPonza aPonza force-pushed the use_clang_parser branch from 9ac9769 to 489e43f Compare March 14, 2020 13:48
@aPonza
Copy link
Contributor Author

aPonza commented Mar 14, 2020

Ok, so it hit the time limit and you can check the log to see the 16 recursion errors. I'm wondering if:

@kunaltyagi
Copy link
Member

kunaltyagi commented Mar 15, 2020

some of the generated errors to see if #3545 solves them and it doesn't seem to do it, yet

No, sorry. That PR took a long time to reach even that stage, and IIRC covers only the files not in the include/common and include/impl folders. Sorry about that. I use GCC (release-latest) and clang (release-latest) so it's getting difficult for me too to fix errors that keep popping up on the CI.

when clearly #include is absolutely needed

I'd go ahead with a PR that fixes some of the header issues. Another thing is if you could provide the name of the docker image (EDIT: it's in the changelog. aponza/doc:experimental) that you've pushed. I'd grab it and start using it as a base to fix the more urgent issues.

@aPonza
Copy link
Contributor Author

aPonza commented Mar 15, 2020

You shouldn't feel sorry, it's a huge undertaking! As you realized it's in one of the commits. I can fix the namespace 'std' stuff tomorrow at least. Let's not overlap.

@aPonza
Copy link
Contributor Author

aPonza commented Mar 15, 2020

Saying this was slow is understating it. And the difference is only 438 lines on ~18k.
output_use_clang_parser.txt
output_use_clang_parser_on_top_of_fix_doc_warnings.txt

I think I'd need to retest if I really had no more recursion warnings and document how so it's repeatable. It might have been an oversight. Not sure this is a useful PR anymore. Is a PR re-openable in case it turns out it is useful? Otherwise I'd close.

@kunaltyagi
Copy link
Member

Is a PR re-openable in case it turns out it is useful?

Yes, unless you delete your branch, in which case, it isn't (but there is a restore button though I haven't tested that)

Saying this was slow is understating it.

You can create a PR with the changes for the 438 line reduction.

Sidenote: did you ask around in doxygen community regarding usage of clang-parsing with compilation database?

@aPonza
Copy link
Contributor Author

aPonza commented Mar 15, 2020

You can create a PR with the changes for the 438 line reduction.

But I have, it's #3701. Depending on how we tackle the recursion warnings it's going to need more/less additions but almost everything else is there iirc.

did you ask around in doxygen

Not yet, I was waiting for these results, will do now.

@aPonza
Copy link
Contributor Author

aPonza commented Mar 29, 2020

Testing Documentation CI run.

@aPonza aPonza force-pushed the use_clang_parser branch from 51afab4 to a1d5185 Compare March 31, 2020 06:54
@aPonza aPonza force-pushed the use_clang_parser branch from a1d5185 to 22d6405 Compare April 9, 2020 16:07
@stale
Copy link

stale bot commented May 19, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label May 19, 2020
@aPonza aPonza force-pushed the use_clang_parser branch from 22d6405 to 0235156 Compare May 19, 2020 15:58
@stale stale bot removed the status: stale label May 19, 2020
@stale
Copy link

stale bot commented Jun 20, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label Jun 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: ci needs: author reply Specify why not closed/merged yet skill: doxygen Skills/areas of expertise needed to tackle the issue status: stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants