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

Add BUILD_DOCS option to enable/disable building documentation. #2730

Merged
merged 4 commits into from Nov 25, 2020

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Nov 22, 2020

This just adds a BUILD_DOCS option to the CMake configuration. Now, if someone is having Doxygen issues, they can disable that from the build by setting -DBUILD_DOCS=OFF. I also updated documentation so that people can know that the option exists.

This is necessary as part of getting Jenkins working correctly again for the Doxygen build and other builds.

@@ -17,6 +17,7 @@ option(DISABLE_DOWNLOADS "Disable downloads of dependencies during build." OFF)
option(DOWNLOAD_ENSMALLEN "If ensmallen is not found, download it." ON)
option(DOWNLOAD_STB_IMAGE "Download stb_image for image loading." ON)
option(BUILD_GO_SHLIB "Build Go shared library." OFF)
option(BUILD_DOCS "Build doxygen documentation (if doxygen is available)." ON)
Copy link
Member

@zoq zoq Nov 22, 2020

Choose a reason for hiding this comment

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

Wondering if the default should be OFF, or if not if we should set WARN_AS_ERROR = NO as default? Until they take a look at: doxygen/doxygen#7818

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point, let's set WARN_AS_ERROR to NO by default, but then sed it to ON in the Doxygen build.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 5552b8c.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good solution to me.

@zoq
Copy link
Member

zoq commented Nov 23, 2020

I use the PR to check if the static analysis job works as expected, turns out the pvs tool expects that #include <filesystem> is supported which is not true for older version of g++, where you have to use #include <experimental/filesystem>. Anyway, I did the change and updated the job config.

@zoq
Copy link
Member

zoq commented Nov 23, 2020

@mlpack-jenkins test this please

@birm birm self-requested a review November 24, 2020 15:28
Copy link
Member

@birm birm left a comment

Choose a reason for hiding this comment

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

Looks good! Hoping that someone eventually works out those doxygen issues so the warnings are just useful.

@zoq
Copy link
Member

zoq commented Nov 24, 2020

I use the PR to check if the static analysis job works as expected, turns out the pvs tool expects that #include <filesystem> is supported which is not true for older version of g++, where you have to use #include <experimental/filesystem>. Anyway, I did the change and updated the job config.

Looks like the format of the output format changed as well, let's see if I fixed it; for some reason the cppcheck jenkins report plugin expects a closing </cppcheck> as well now.

@zoq
Copy link
Member

zoq commented Nov 24, 2020

@mlpack-jenkins test this please

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Looks great to me.

@jeffin143 jeffin143 merged commit 047f881 into mlpack:master Nov 25, 2020
This was referenced Oct 14, 2022
@rcurtin rcurtin mentioned this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants