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.

Sign up for GitHub

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

Jump to bottom

Run doxygen layout checker in prepare-commit script, when possible #54936

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Run doxygen layout checker in prepare-commit script, when possible #54936

merged 1 commit into from
Oct 23, 2023

Conversation

strk
Copy link
Contributor

@strk strk commented Oct 14, 2023

@strk strk added the Chore GitHub and other CI infrastructure changes label Oct 14, 2023
@github-actions github-actions bot added this to the 3.34.0 milestone Oct 14, 2023
@nyalldawson
Copy link
Collaborator

The doxygen check requires that the non-default CMAKE option "WITH_APIDOC" is set. This isn't set by default because it slows down the build considerably (especially for otherwise trivial rebuilds).

I'm not sure if it's possible, but the prepare-commit script should only run the check if WITH_APIDOC is true...

@strk
Copy link
Contributor Author

strk commented Oct 16, 2023

The tests/code_layout/test_doxygen_layout.sh does not depend on cmake at all, and it is supposed to act on the source tree, not the build tree (whereas you can have multiple build trees with multiple configurations) so I don't see why the script should deal with cmake configuration at all.

I've rebased the work and added support in the test_doxygen_layout.sh to receive a list of files to test so the cost of check should now be reduced as only modified files are checked.

@strk
Copy link
Contributor Author

strk commented Oct 16, 2023

@nyalldawson did you have a chance to test this ? On my system it is really fast to check doxygen layout now as it only checks actually modified files and only if they have a .h extension

lnicola
lnicola reviewed Oct 17, 2023
View reviewed changes
scripts/prepare_commit.sh Outdated Show resolved Hide resolved
@strk
Run doxygen layout checker in prepare-commit script, when possible
598ee21 
Helps preventing useless CI wait like in here:
https://github.com/qgis/QGIS/actions/runs/6518501862/job/17703989815?pr=54934

Allow passing list of files to check to test_doxygen_layout.sh script
@strk strk requested a review from lnicola October 19, 2023 08:49
lnicola
lnicola approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

Untested, but looks good to me.

@nyalldawson
Copy link
Collaborator

@strk
Sorry -- I was getting confused with https://github.com/qgis/QGIS/blob/master/tests/code_layout/test_qgsdoccoverage.py

@nyalldawson nyalldawson merged commit 07345e5 into qgis:master Oct 23, 2023
28 checks passed
@strk strk deleted the doxygen-in-prepare-commit branch October 23, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lnicola lnicola lnicola approved these changes

@nyalldawson nyalldawson nyalldawson approved these changes

Assignees
No one assigned
Labels
Chore GitHub and other CI infrastructure changes
Projects
None yet
Milestone
3.34.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@strk @nyalldawson @lnicola