Merged
Conversation
4210702 to
9eb5882
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR:
seqan-stda CMake projectseqan-stdcan now be added via CPMIn #9, I also added CI. I did have to make a separate PR for that to make the actions run on this PR.
It is a simple compile/header test:
Follow-up
I think I'll also change the test's
main.cppto include all headers in a follow-up PR. This way, we would catch if some headers have the same include guards.I'll also try to add a test that checks whether the
FILE_SET HEADERSdoes indeed contain all headers.Comments
Why
Instead of adding an
include/seqan-stddirectory, I opted for:That is, the parent directory is the include directory. It allows for, e.g.
I haven't found any sources on whether this is considered bad style or not.
Alternative 1
I also tried to use an
include/seqan-stddirectory. For CPM-usage that works just as fine.The main disadvantage is that it's not trivial to use
seqan-stdas git subtree inseqan3andhibf.The includes would need to be
We would need to add
seqan3/contrib/seqan-stdto the include path.I don't like either option much.
By keeping the headers in the top-level directory, we don't need to change anything about the git subtree usage.
Alternative 2
An alternative would be to just include
seqan-stdvia CPM.But because we do not install
seqan-std, we would have to ship it in our source packages or when installing seqan3/hibf.Here is what this would look like: eseiler/seqan3@713ca0e
And here is a patch for posterity: seqan3_use_seqan-std_via_CPM.patch
While it works, it's far too much complexity.