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

Feature/extend build dependency #80

Merged

Conversation

ClausKlein
Copy link
Contributor

@ClausKlein ClausKlein commented Feb 10, 2021

add build time dependency to static lib Greeter

this also prevents this #79 issue

the header only fmt lib is used to show this
doctest_discover_tests() is only availabe if doctest is not imported with
find_packag()
@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #80 (b575afd) into master (b58e071) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #80   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           12        12           
=========================================
  Hits            12        12           
Impacted Files Coverage Δ
source/greeter.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b58e071...b575afd. Read the comment docs.

@ClausKlein ClausKlein changed the title Feature/extent build dependency Feature/extend build dependency Feb 11, 2021
Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR! I do like the idea of adding an example C++ dependency to the main library. Some points should be addressed though, as I'd like to keep the starter as simple and general as possible. Note that a lot is personal preference, which is why we encourage users to create their own forks with their personal modifications.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
no default cxx language settings
install fmt
link against fmt::fmt
export dependency too if not needed
...
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
standalone/source/main.cpp Outdated Show resolved Hide resolved
standalone/source/main.cpp Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/source/greeter.cpp Outdated Show resolved Hide resolved
ClausKlein and others added 2 commits February 16, 2021 12:42
Co-authored-by: Lars Melchior <TheLartians@users.noreply.github.com>
update to current PackageProject version 1.5.0 too
Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Changes looking good, thanks! Some small things remain.

standalone/source/main.cpp Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
ClausKlein and others added 2 commits February 18, 2021 09:26
Co-authored-by: Lars Melchior <TheLartians@users.noreply.github.com>
remove NOLINT comment
needs to use doctest cmake modules ...
it is not my idea!
Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the changes! I think having an example dependency in the main CMakeLists is a great improvement for the starter!

@TheLartians TheLartians merged commit 2db60f2 into TheLartians:master Feb 19, 2021
@ClausKlein ClausKlein deleted the feature/extent-build-dependency branch February 21, 2021 06:39
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