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

Enable Doxygen Documentation Generation #316

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

Conversation

jflopezfernandez
Copy link
Collaborator

This pull request enables the automatic generation of Doxygen HTML documentation via the check-doxygen-docs build target.

This build target is not included in the Check library default build list, as requested in issue #217, but instead allows the user to specifically build this documentation after project generation with the following command.

$ cmake --build . --target check-doxygen-docs

The generated documentation is created in the project build directory, in the doc/Doxygen folder.

In order to enable Doxygen, the project must be configured using the -DCHECK_ENABLE_DOXYGEN_DOCS CMake flag.

As stated in the CMakeLists.txt file, the CMake FindDoxygen module does not require a Doxyfile. Instead, one is automatically generated based on the global state at the time the doxygen_add_docs command is called. I did not remove any of the Doxygen configuration files in the project directory, however, so that we can have the original values for both of the existing configuration files to configure the generated Doxygen docs should this pull request be approved.

This pull request enables the automatic generation of
Doxygen HTML documentation via the check-doxygen-docs build
target.

This build target is not included in the Check library
default build list, as requested in issue libcheck#217, but instead
allows the user to specifically build this documentation
after project generation with the following command.

 $ cmake --build . --target check-doxygen-docs

The generated documentation is created in the project build
directory, in the doc/Doxygen folder.

In order to enable Doxygen, the project must be configured
using the -DCHECK_ENABLE_DOXYGEN_DOCS CMake flag.
@jflopezfernandez
Copy link
Collaborator Author

Hey, @mikkoi, can you take a look at this?

Copy link
Contributor

@brarcher brarcher left a comment

Choose a reason for hiding this comment

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

As this adds a new build option, probably there should be a separate test for it in Github Actions that exercises it. Which platform it builds for probably is not that important. Should it be tested for Linux, macOS, and Windows, or just run for Linux (for example)? What is your suggestion?

@jflopezfernandez
Copy link
Collaborator Author

Of those three, I'm definitely more familiar with Linux, so I could much more easily diagnose what problems are caused by the platform, the buildsystem, or Doxygen itself. I would prefer Linux, personally.

@brarcher
Copy link
Contributor

Having it run on Linux only sounds fine.

@jflopezfernandez
Copy link
Collaborator Author

Hey, Branden, I only just realized today you'd replied, sorry about that.

I still need to push a commit to actually install the generated documentation. I see your review suggested changes, but I can't actually find any. Is it adding the tests to the build scripts?

@brarcher
Copy link
Contributor

My suggestion is adding something to the GitHub actions here:
https://github.com/libcheck/check/blob/master/.github/workflows/linux.yml
that would pass the correct flags to build the documentation.

@jflopezfernandez
Copy link
Collaborator Author

Gotcha. Yea, I haven't gotten to mess around with the GitHub actions yet, that's the next thing I'll do once I get the configuration working properly. I haven't really interacted with the GitHub API since they made the switch to GraphQL, so it looks like I have some documentation to pore over this weekend.

@jflopezfernandez
Copy link
Collaborator Author

Hey @brarcher Branden, is there a way to test changes to the workflows? I'm experimenting with the matrix strategy build, and this is what I have so far.

name: Check CMake Buildsystem CI Workflow

on:
  push:
    branches: [ $default-branch ]
  pull_request:
    branches: [ $default-branch ]

jobs:
  build:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        config: [ Debug, Release ]
        os: [ macos-latest, ubuntu-latest ]
    steps:
    - uses: actions/checkout@v2
    - name: configure
      run: cmake . -DCMAKE_BUILD_TYPE=${{ matrix.config }}
    - name: make
      run: cmake --build . --config ${{ matrix.config }}
    - name: install
      run: cmake --install . --config ${{ matrix.config }} 

I haven't added checks to verify the installation of the documentation yet, but I feel like these revisions make for a more robust build, what do you think?

@brarcher
Copy link
Contributor

Ah, so you are interested in changing the organization from grouping the tests by OS to grouping by build system? That is fine. Would there be these files under check/.github/workflows

autotools.yml
cmake.yml

where the various Windows's toolchains would have various entries among these files? If so, that sounds reasonable.

@jflopezfernandez
Copy link
Collaborator Author

Potentially, although to be completely honest, I was kind of just throwing some stuff around to get the hang of GitHub's actions system. I really like the strategy matrix idea, and I also really like the potential for really clean build configurations, but most of the examples were geared more towards Node development, so it's very much still just an idea.

Are you just supposed to push the workflows to master and trigger some perfunctory change on the master branch to see if everything works? I found this tool to help speed up the process which looks pretty cool, although I haven't tried it yet.

@jflopezfernandez
Copy link
Collaborator Author

The only reason I didn't include Windows was because you have to pass in the toolset option -T x64 when you're configuring, right? I haven't done it in a while via the command line, and since I was trying to see if I could test it quickly last night, Windows ended up falling by the wayside. Ideally, however, there wouldn't need to be a separate Windows toolchain file, since we should be able to run a simple operating-and-build-type-dependent CMake configuration command.

@brarcher
Copy link
Contributor

Are you just supposed to push the workflows to master and trigger some perfunctory change on the master branch to see if everything works?

If you add a commit in the PR, it will build the PR with the updated workflows. When I added the workflows I would add a commit to a PR, check the result, and iterate until it worked. Then merge the changes into the master branch.

RE: Windows, there are several different toolchains, and they need to be used differently. The existing workflow for Windows shows the examples I've been able to get working so far. For CMake builds on Windows CMake and output build files for a few different compiler toolchains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants