Skip to content

tests: Remove posix exclude #19507

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
Merged
merged 2 commits into from
Jan 4, 2020
Merged

tests: Remove posix exclude #19507

merged 2 commits into from
Jan 4, 2020

Conversation

@finikorg
Copy link
Contributor

@finikorg finikorg commented Oct 1, 2019

Build also for posix.

@finikorg finikorg requested a review from nashif as a code owner October 1, 2019 10:26
@aescolar aescolar requested a review from pabigot October 1, 2019 10:29
@aescolar aescolar added area: C++ area: Tests Issues related to a particular existing or missing test labels Oct 1, 2019
pabigot
pabigot reviewed Oct 1, 2019
View reviewed changes
Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

I can't see any record of the motivation for excluding this originally. It may have been a toolchain issue that's been fixed.

I don't see any problem with this, but would like @vanwinkeljan who's done cross-platform/toolchain testing more recently.

Also if there is no reason to exclude posix then the same change should be made to the libcxx test.

pabigot
pabigot suggested changes Oct 1, 2019
View reviewed changes
Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

Never mind. Looking at the shippable results posix is excluded because shippable won't pass due to a lack of libstdc++.

@finikorg finikorg mentioned this pull request Oct 1, 2019
Closed
@vanwinkeljan
Copy link
Member

vanwinkeljan commented Oct 1, 2019

Never mind. Looking at the shippable results posix is excluded because shippable won't pass due to a lack of libstdc++.

Is this not actually due to a missing 32-bit version (multilib) of stdc++ in the ci docker container

See: zephyrproject-rtos/ci-dockerfiles#19

@pabigot pabigot added the Blocked Blocked by another PR or issue label Oct 1, 2019
@aescolar
Copy link
Member

aescolar commented Oct 1, 2019

I can't see any record of the motivation for excluding this originally

When native_posix was first added, C++ did not work on it at all. There has been many fixes over this ~2 years that should have made it possible now.

@pabigot pabigot mentioned this pull request Oct 8, 2019
Merged
@pabigot pabigot mentioned this pull request Oct 18, 2019
Closed
@pabigot pabigot removed the Blocked Blocked by another PR or issue label Oct 31, 2019
@pabigot
Copy link
Contributor

pabigot commented Oct 31, 2019

@finikorg Could you rebase this so we can verify the merge of #19735 enables this to pass shippable?

@finikorg
Copy link
Contributor Author

finikorg commented Nov 4, 2019

@finikorg Could you rebase this so we can verify the merge of #19735 enables this to pass shippable?

Done

@vanwinkeljan vanwinkeljan added the Blocked Blocked by another PR or issue label Nov 18, 2019
@vanwinkeljan
Copy link
Member

vanwinkeljan commented Nov 18, 2019 •
edited
Loading

Added label blocked as CI docker image update is needed see zephyrproject-rtos/ci-dockerfiles#59

vanwinkeljan
vanwinkeljan suggested changes Nov 18, 2019
View reviewed changes
Copy link
Member

@vanwinkeljan vanwinkeljan left a comment

Choose a reason for hiding this comment

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

Please also remove arch_exclude: posix from tests/application_development/libcxx/testcase.yaml.

@finikorg
Copy link
Contributor Author

finikorg commented Nov 18, 2019

Please also remove arch_exclude: posix from tests/application_development/libcxx/testcase.yaml.

added second patch

vanwinkeljan
vanwinkeljan approved these changes Nov 18, 2019
View reviewed changes
Copy link
Member

@vanwinkeljan vanwinkeljan left a comment

Choose a reason for hiding this comment

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

thx, now we only have to wait for a new CI docker image including multilib and C++ 17

@vanwinkeljan vanwinkeljan requested a review from pabigot November 18, 2019 08:41
@vanwinkeljan
Copy link
Member

vanwinkeljan commented Dec 12, 2019

blocked by #20625

@vanwinkeljan vanwinkeljan removed the Blocked Blocked by another PR or issue label Dec 19, 2019
@vanwinkeljan
Copy link
Member

vanwinkeljan commented Dec 19, 2019

@finikorg could you rebase with latest master?

@finikorg
tests: Remove posix exclude
3655bfa 
Build also for posix.

Signed-off-by: Andrei Emeltchenko <[email protected]>
@finikorg
tests: Remove posix arch_exclude for libcxx
42f7c27 
Remove arch_exclude: posix also for
tests/application_development/libcxx.

Signed-off-by: Andrei Emeltchenko <[email protected]>
@finikorg
Copy link
Contributor Author

finikorg commented Dec 23, 2019

@finikorg could you rebase with latest master?

Done.

@vanwinkeljan
Copy link
Member

vanwinkeljan commented Dec 23, 2019

@pabigot can please revisit you review

@zephyrbot zephyrbot closed this Jan 3, 2020
@zephyrbot zephyrbot reopened this Jan 3, 2020
@vanwinkeljan
Copy link
Member

vanwinkeljan commented Jan 4, 2020

@nashif can this be merged?

@nashif nashif merged commit e742637 into zephyrproject-rtos:master Jan 4, 2020
@finikorg finikorg deleted the posix branch September 6, 2021 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nashif nashif Awaiting requested review from nashif

2 more reviewers

@pabigot pabigot pabigot approved these changes

@vanwinkeljan vanwinkeljan vanwinkeljan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: C++ area: Tests Issues related to a particular existing or missing test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@finikorg @vanwinkeljan @aescolar @pabigot @nashif @zephyrbot