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

Fix CI by including <algorithm> to stop MSVC from complaining about std::count_if in tests/test_sequences_and_iterators.cpp #2435

Conversation

YannickJadoul
Copy link
Collaborator

No description provided.

…td::count_if in tests/test_sequences_and_iterators.cpp
@henryiii henryiii merged commit 03b3d59 into pybind:master Aug 26, 2020
@henryiii
Copy link
Collaborator

PS: MSVC is correct, we should not be depending on stdlib includes including other stdlib headers.

@YannickJadoul YannickJadoul deleted the fix-ci-test_sequences_and_iterators-algorithm branch August 26, 2020 09:51
@xkszltl
Copy link
Contributor

xkszltl commented Sep 15, 2020

Can we have a new release tag with this, or back-port it to previous releases?
Without this patch the current latest release simply doesn't work on Windows.

@henryiii
Copy link
Collaborator

current latest release simply doesn't work on Windows

A fix to /tests? We are moving toward a release at some point, but I don't think back porting a fix to the tests affects anyone except a developer building pybind11's test suite...

@xkszltl
Copy link
Contributor

xkszltl commented Sep 15, 2020

Test is useful when build and packag pybind as deb/rpm/nuget/docker.....
For whatever we build, we always run its test to make sure:

  1. It can pass the test in our environment
  2. There's no missing .so or .dll at runtime.
    Without running unit test, we cannot tell whether the built binary works or not.

@henryiii
Copy link
Collaborator

There's no missing .so or .dll at runtime

This is header-only, so there is no binary to build.

deb/rpm

These at least are not Windows, and are unaffected. (Docker, mostly, too)

If you want to run the test, grab https://github.com/pybind/pybind11/pull/2435.patch and apply it in the build recipe. Most of those should also have already created packages. Otherwise, expect a release in the Near Future™️ (really, it shouldn't be too far off).

I'm hoping after 2.6.0 it will be much easier to make a quick patch release if needed.

@xkszltl
Copy link
Contributor

xkszltl commented Sep 15, 2020

Yes that's what I'm currently doing, git cherry-pick xxx.
I'm fine with waiting for the next release if it's not far away.
Just bring up our general practice when build/package things (not only pybind) to give some idea about why unit test can still be useful for people outside of the project.
And the suggestion of back-porting is mainly for other people, we've already added the cherry-pick to our script:
https://github.com/xkszltl/Roaster/blob/8d37127e9ca0d714bcd847d71da8f25a51413282/win/pkgs/pybind11.ps1#L31

@henryiii
Copy link
Collaborator

Perfect! Since it's only a test modification, there is no harm in adding it, it's still "2.5.0". Hopefully patch releases will be much easier soon. :)

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.

4 participants