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

Remove adapters #4

Merged
merged 6 commits into from
Oct 22, 2020
Merged

Remove adapters #4

merged 6 commits into from
Oct 22, 2020

Conversation

jlconlin
Copy link
Member

@jlconlin jlconlin commented Oct 22, 2020

This removes the need for spdlog-adapter and fmt-adapter.

This also adds the unit testing back in, which was previously removed.

@jlconlin jlconlin changed the base branch from master to build/fetchcontent October 22, 2020 16:48
@jlconlin jlconlin linked an issue Oct 22, 2020 that may be closed by this pull request
2 tasks
@nathangibson14
Copy link
Contributor

This doesn't have any changes related to the adapters. Did you forget to push a commit that changed the cmake files?

@jlconlin
Copy link
Member Author

Sorry, I forgot one last push. It should all be there now.

Copy link
Contributor

@nathangibson14 nathangibson14 left a comment

Choose a reason for hiding this comment

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

This does compile just fine, but you've ended up adding spdlog's test suite into the list of targets. Do you know if those can be disabled? I'm guessing this would actually include the spdlog tests as targets for all upstream repositories as well, which isn't our normal process.

GIT_TAG origin/build/fetchcontent
FetchContent_Declare( spdlog
GIT_REPOSITORY https://github.com/gabime/spdlog.git
GIT_TAG a51b4856377a71f81b6d74b9af459305c4c644f8
GIT_SHALLOW TRUE
Copy link
Contributor

Choose a reason for hiding this comment

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

This will get fixed later, but FYI, you don't need GIT_SHALLOW when GIT_TAG points to a hash or tag. It only has meaning for branches.

Copy link
Member Author

Choose a reason for hiding this comment

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

You beat me to it. I was about to do the same thing.

@nathangibson14
Copy link
Contributor

I fixed it and pushed. I'll have to debate as to whether the commands make sense where I put them or if I should have put them in the main CMakeLists.txt file. But this will work for now.

@jlconlin
Copy link
Member Author

We still have the spdlog-utests being built.

@nathangibson14
Copy link
Contributor

It worked for me before, but not now -- weird. I think I can fix it.

@jlconlin jlconlin merged commit 3860198 into build/fetchcontent Oct 22, 2020
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.

Remove need for adapters.
2 participants