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

Fix ODR violations in our Eigen Tensor tests #4412

Merged
merged 3 commits into from
Dec 20, 2022
Merged

Fix ODR violations in our Eigen Tensor tests #4412

merged 3 commits into from
Dec 20, 2022

Conversation

EthanSteinberg
Copy link
Collaborator

Description

The current way we are doing unit tests with Eigen unintentionally triggers ODR violations due to testing both with and without std::array.

This fixes those ODR violations by updating our build scripts.

@EthanSteinberg EthanSteinberg changed the title Fix ODR violations in our Eigen Tensor Tests Fix ODR violations in our Eigen Tensor tests Dec 17, 2022
@EthanSteinberg EthanSteinberg marked this pull request as draft December 17, 2022 13:58
@EthanSteinberg
Copy link
Collaborator Author

Hi @rwgk, can you take a look at this PR? I think this fixes the ODR issue in our tests.

At least, based on https://github.com/pybind/pybind11/actions/runs/3720383627/jobs/6309827070

@EthanSteinberg EthanSteinberg marked this pull request as ready for review December 17, 2022 14:23
rwgk
rwgk reviewed Dec 19, 2022
View reviewed changes
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

I'm traveling and have only very unreliable internet at the moment, therefore I couldn't try things out myself. I'll look more as soon as I can.

tests/test_eigen_tensor.inl Show resolved Hide resolved
tests/test_eigen_tensor.cpp Outdated Show resolved Hide resolved
tests/test_eigen_tensor_avoid_stl_array.cpp Show resolved Hide resolved
@rwgk rwgk mentioned this pull request Dec 20, 2022
Merged
rwgk
rwgk approved these changes Dec 20, 2022
View reviewed changes
Copy link
Collaborator

@rwgk rwgk 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 tweaks! With that I just had to add one line in my SConscript to adjust to this PR.

@rwgk rwgk merged commit ee4b9f5 into pybind:master Dec 20, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 20, 2022
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label Dec 20, 2022
@EthanSteinberg EthanSteinberg deleted the fix_eigen_test_odr_violations branch December 20, 2022 13:34
@rwgk rwgk mentioned this pull request Feb 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwgk rwgk rwgk approved these changes

@Skylion007 Skylion007 Skylion007 approved these changes

@henryiii henryiii Awaiting requested review from henryiii henryiii is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@EthanSteinberg @rwgk @Skylion007