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 hotdoc #41

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

thiblahute
Copy link
Contributor

@thiblahute thiblahute commented Jan 6, 2019

This is the very basic stub to generate the documentation with the hotdoc C extension (requires llvm). Currently there is basically not doc string so the documentation is very sparse!

You can find the current output here: https://people.igalia.com/tsaunier/documentation/libwpe/c-index.html

Fixes #37

Copy link
Contributor

@aperezdc aperezdc 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 overall, but please check the comment.

CMakeLists.txt Outdated
@@ -107,3 +107,23 @@ install(
DESTINATION
${CMAKE_INSTALL_LIBDIR}/pkgconfig
)

execute_process(
COMMAND hotdoc --has-extension c-extension
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me that it would be good to use find_program() to know whether the hotdoc tool is available before trying to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I would prefer that building the documentation is controlled by an option() variable, instead of automatically deciding whether it will be built depending on the availability of the hotdoc and its C extension. That means that, if the option to build the documentation is enabled, if the hotdoc program or its C extension are missing, configuring with CMake should fail instead of silently disabling the documentation (“Explicit is better than implicit”, after all 😉).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks to me that it would be good to use find_program() to know whether the hotdoc tool is available before trying to use it.

DONE, it sensibly enhance the debug message but considered it useless first, anyway if you prefer.

Also, I would prefer that building the documentation is controlled by an option() variable, instead of automatically deciding whether it will be built depending on the availability of the hotdoc and its C extension. That means that, if the option to build the documentation is enabled, if the hotdoc program or its C extension are missing, configuring with CMake should fail instead of silently disabling the documentation (“Explicit is better than implicit”, after all wink).

DONE.

@aperezdc
Copy link
Contributor

aperezdc commented Jan 7, 2019

Mmmh, is there something missing, like for example the configuration files for HotDoc? I tried applying the commit locally and all that HotDoc generates for me is an empty HTML 🤔

@thiblahute
Copy link
Contributor Author

Mmmh, is there something missing, like for example the configuration files for HotDoc? I tried applying the commit locally and all that HotDoc generates for me is an empty HTML thinking

No it doesn't need a config file because I pass everything needed as arguments. Could you show me all the logs?

I just cloned and tried from a clean build on my build machine and everything works just fine here.

Copy link
Contributor

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

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

This is looking almost perfect, thanks for the update! Would you mind doing a coupl of changes more to ensure cross-compilation is handled as best as possible?

CMakeLists.txt Outdated
@@ -107,3 +107,30 @@ install(
DESTINATION
${CMAKE_INSTALL_LIBDIR}/pkgconfig
)

option(DOCUMENTATION "Build the documentation" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we call this BUILD_DOCS instead? It seems to be the name that most CMake projects use as a convetion, and some build systems automatically pass -DBUILD_DOCS= to CMake automatically (Buildroot, for example).

Choose a reason for hiding this comment

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

Done, I do not know anything about CMake and the conventions around it, thanks for letting me know! :-)

CMakeLists.txt Outdated

option(DOCUMENTATION "Build the documentation" OFF)
IF(DOCUMENTATION)
find_program(HAS_HOTDOC hotdoc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking the docs for find_program() I understand that the HAS_HOTDOC variable will be set to the full path of the program, when found. I think this would be nicer as follows:

    find_program(HOTDOC hotdoc)
    if (NOT HOTDOC)
        message(FATAL_ERROR "hotdoc not found!")
    endif()

    execute_process(
        COMMAND ${HOTDOC} --has-extension c-extension
        RESULT_VARIABLE HAS_HOTDOC_C_EXTENSION
    )
    # ...

Note how the ${HOTDOC} variable is used, this way the path detected by CMake is being used. This can be important (e.g. when cross-building) because the binary to run may not be the one in one of the locations from the PATH environment variable.

Choose a reason for hiding this comment

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

Done.

CMakeLists.txt Outdated
message(FATAL_ERROR "hotdoc not found!")
ENDIF()

execute_process(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the indentation.

Choose a reason for hiding this comment

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

Done

@tsaunier
Copy link

Hope we are good now. Thanks for the review!

@aperezdc
Copy link
Contributor

This looks all good now, thanks a lot for the effort @thiblahute! I am still having trouble to get the generated files to have any content at all, but I'll blame that in the Arch Linux package for HotDoc this time.

Nevertheless, before merging I would like to try using HotDoc with a Fedora chroot (is that the distro you use?) or at least a virtualenv to make sure that the problem is on my side, so please give me a couple of days before hitting the “Rebase and merge” button 😉

@thiblahute
Copy link
Contributor Author

You should pip install installing required dependencies as described here: https://hotdoc.github.io/installing.html

I am using Fedora these days, but used to use Arch (a few month ago) and hotdoc was working well there.

@zdobersek
Copy link
Contributor

Thanks for setting this up.

Do we want to build up more meaningful documentation in a separate branch, or just do it in master?

@aperezdc
Copy link
Contributor

@thiblahute It's working here now as well 👍

@zdobersek I think we can merge this to have the documentation building, and incrementally add documentation in follow-up PRs. Documentation can be a good starting point for new contributors, so I think we can leave #37 open and marked as “good first bug” after this is merged (or add a new issue) to make people aware that the actual documentation is still missing.

Additionally, we can start asking in reviews for new PRs to have documentation for the public API, if they add to it. WDYT?

@zdobersek
Copy link
Contributor

SGTM.

@zdobersek zdobersek merged commit 734f2ac into WebPlatformForEmbedded:master Jan 21, 2019
@aperezdc aperezdc added this to the Version 1.2.0 milestone Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants