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

Update range library to 0.10.0 #1471

Merged
merged 3 commits into from
Jan 17, 2020
Merged

Update range library to 0.10.0 #1471

merged 3 commits into from
Jan 17, 2020

Conversation

marehr
Copy link
Member

@marehr marehr commented Jan 8, 2020 •
edited
Loading

Fixes #1479

@marehr marehr requested a review from MitraDarja January 8, 2020 17:08
@mr-c
Copy link
Contributor

mr-c commented Jan 10, 2020

This is appreciated, as Debian has already upgraded to Range 0.10.0

@marehr marehr mentioned this pull request Jan 11, 2020
Closed
@marehr marehr requested review from eseiler and removed request for MitraDarja January 13, 2020 12:19
eseiler
eseiler requested changes Jan 13, 2020
View reviewed changes
include/seqan3/std/span
@@ -54,6 +54,25 @@ struct is_std_array_impl<array<span_tp, span_sz>> : public true_type {};
template <class span_tp>
struct is_std_array : public is_std_array_impl<remove_cv_t<span_tp>> {};

} // namespace std

#if RANGE_V3_VERSION >= 901
Copy link
Member

Choose a reason for hiding this comment

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

We require >= 9.0.0. I guess you mean >= 900?
But then we wouldn't need this #if since platform.hpp already checks it

Copy link
Member Author

Choose a reason for hiding this comment

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

no this was introduced between 9.0.1 and 10.0.0

@marehr marehr requested a review from rrahn January 13, 2020 14:45
@codecov
Copy link

codecov bot commented Jan 13, 2020 •
edited
Loading

Codecov Report

Merging #1471 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1471   +/-   ##
=======================================
  Coverage   97.58%   97.58%           
=======================================
  Files         235      235           
  Lines        8844     8844           
=======================================
  Hits         8630     8630           
  Misses        214      214
Impacted Files Coverage Δ
include/seqan3/search/algorithm/search.hpp 86.95% <0%> (ø) ⬆️

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 af35977...142795a. Read the comment docs.

@marehr marehr force-pushed the update_range_to_0.10.0 branch 2 times, most recently from 35f9f93 to 2abd3d1 Compare January 13, 2020 15:14
rrahn
rrahn requested changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

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

Minor visual thing

CHANGELOG.md Outdated Show resolved Hide resolved
@marehr marehr requested a review from rrahn January 14, 2020 13:06
@rrahn
Copy link
Contributor

rrahn commented Jan 16, 2020

@marehr you can squash the last commit and we can merge.

@h-2
Copy link
Member

h-2 commented Jan 16, 2020

Just as a note: If you advertise that you support both 0.9.x and 0.10.x you need to test both branches individually, too. (there is no stable API right now)

I would suggest just moving to the new version and as soon as a stable version is released, use that.

@rrahn
Copy link
Contributor

rrahn commented Jan 16, 2020

So you suggest dropping support for 0.9. altogether?

@h-2
Copy link
Member

h-2 commented Jan 16, 2020

Yep! The interfaces between range-v3 versions are unstable anyway (until 1.0). As long as SeqAn3 requires range-v3 (no std::ranges yet), I don't see why it needs to provide backwards compatibility. And like I said, supporting multiple versions means having to test basically all tests with both.

@marehr
Copy link
Member Author

marehr commented Jan 16, 2020

@rrahn done

@marehr marehr requested a review from rrahn January 16, 2020 22:07
@smehringer
Copy link
Member

I would also only advertise the version we are testing.

@marehr
[MISC] Upgrade range-v3 submodule to 0.10.0 and increase supported ve…
ed4f841 
…rsion of ranges library to >=0.10.0 and < 0.11.0
@marehr
Copy link
Member Author

marehr commented Jan 17, 2020

@smehringer @h-2 @rrahn I upped the minimum requirement

smehringer
smehringer requested changes Jan 17, 2020
View reviewed changes
CHANGELOG.md Outdated
@@ -45,6 +45,8 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.

## API changes

* **The required version of the ranges-v3 library has increased:** We now support the versions >= 0.9.0 and < 0.11.0,
Copy link
Member

Choose a reason for hiding this comment

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

The documentation needs also an update :)

README.md Outdated
@@ -35,7 +35,7 @@ Please see the [online documentation](https://docs.seqan.de/seqan/3-master-user/
|**compiler** | [GCC](https://gcc.gnu.org) | ≥ 7 | no other compiler is currently supported! |
|**build system** | [CMake](https://cmake.org) | ≥ 3.4 | optional, but recommended |
|**required libs** | [SDSL](https://github.com/xxsds/sdsl-lite) | ≥ 3 | |
| | [Range-V3](https://github.com/ericniebler/range-v3) | ≥ 1.0 | |
| | [Range-V3](https://github.com/ericniebler/range-v3) | ≥ 0.9.0 | |
Copy link
Member

Choose a reason for hiding this comment

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

here too

@smehringer
Copy link
Member

@smehringer @h-2 @rrahn I upped the minimum requirement

Thanks, although I don't know what @rrahn votes for. But if we want we can still add nightlies for the other version and change the requirements again I think.

@rrahn
Copy link
Contributor

rrahn commented Jan 17, 2020

Thanks, although I don't know what @rrahn votes for. But if we want we can still add nightlies for the other version and change the requirements again I think.

No, please not. I don't know what you think, but I am in total favor of having as least dependencies as possible.

@smehringer smehringer merged commit e22a79c into seqan:master Jan 17, 2020
@marehr marehr deleted the update_range_to_0.10.0 branch January 17, 2020 21:17
@marehr marehr mentioned this pull request Feb 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mr-c mr-c mr-c approved these changes

@rrahn rrahn rrahn approved these changes

@smehringer smehringer smehringer approved these changes

@eseiler eseiler eseiler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build fails: latest seqan3 dev + rangv3 0.10 on Debian dev
6 participants
@marehr @mr-c @rrahn @h-2 @smehringer @eseiler