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

fix: CMake package and pkgconfig rework #916

Merged
merged 26 commits into from
Sep 15, 2023
Merged

fix: CMake package and pkgconfig rework #916

merged 26 commits into from
Sep 15, 2023

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Aug 15, 2023

add tests that exercise the CMake module and pkg-config files

@codecov
Copy link

codecov bot commented Aug 15, 2023 •
edited

Codecov Report

Patch has no changes to coverable lines.

📢 Thoughts on this report? Let us know!.

@phlptp
Copy link
Collaborator Author

phlptp commented Aug 18, 2023

FIxes #911

phlptp
phlptp commented Aug 18, 2023
View reviewed changes
.github/workflows/tests.yml
@@ -35,7 +35,7 @@ jobs:
cmake -S . -B build \
-DCMAKE_CXX_STANDARD=${{matrix.std}} \
-DCLI11_SINGLE_FILE_TESTS=OFF \
-DCLI11_EXAMPLES=OFF \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this change is what caused the coverage change, but I don't think the examples should be built or tested during the single file tests, not entirely sure what is going on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Coverage upload is finicky, it's possible it just had too many jobs not able to report. Going to try a rebuild to see. Otherwise, not too worked about it.

Copy link
Collaborator Author

@phlptp phlptp Sep 15, 2023 •
edited

Choose a reason for hiding this comment

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

I should take another crack at getting back to 100% sometime soon. Probably not in this PR though

Copy link
Collaborator

Choose a reason for hiding this comment

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

The failing lines are

https://github.com/phlptp/CLI11/blob/1cb75471863bb364038de3a5ef8b596694945990/include/CLI/impl/App_inl.hpp#L608-L609

https://github.com/phlptp/CLI11/blob/1cb75471863bb364038de3a5ef8b596694945990/include/CLI/impl/Config_inl.hpp#L250-L251

https://github.com/phlptp/CLI11/blob/1cb75471863bb364038de3a5ef8b596694945990/include/CLI/impl/Config_inl.hpp#L342-L343

I think they were uncovered before, but for some reason they are now counting as a slightly higher percentage? Coverage is weird unless it's 100%. :/

Not worried about it for this PR.

@phlptp phlptp requested a review from henryiii August 18, 2023 03:08
@henryiii henryiii closed this Sep 15, 2023
@henryiii henryiii reopened this Sep 15, 2023
@henryiii henryiii changed the title add tests of the CMake package and pkgconfig fix: CMake package and pkgconfig rework Sep 15, 2023
@henryiii henryiii merged commit c071cb6 into CLIUtils:main Sep 15, 2023
73 of 74 checks passed
@github-actions github-actions bot added needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Sep 15, 2023
@Porkepix Porkepix mentioned this pull request Feb 5, 2024
Merged
@j-rivero j-rivero mentioned this pull request Feb 26, 2024
Merged
phlptp pushed a commit that referenced this pull request Mar 11, 2024
@j-rivero
Restore non-arch dependant path for the pkgconfig file (#1012)
852fc94 
Originally setup at #635 but
reverted in #916 without any
specific reason that I can find.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henryiii henryiii Awaiting requested review from henryiii

Assignees
No one assigned
Labels
needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@phlptp @henryiii