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

Additional includes into test/support/test_config.h #1548

Merged

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Apr 30, 2024

In this PR we have 3 big changes:

  1. In the file test/support/test_config.h we add inclusion of some files from standard library to have correct definitions of used defines:
// Any include from standard library required to have correct state of _GLIBCXX_RELEASE
#if __has_include(<version>)
#   include <version>
#else
#   include <ciso646>
#endif
  1. In all tests now we include test/support/test_config.h at first place.
    No additional includes required anymore if they are really not required for the test: example1, example2
  2. For CI errors after (1) and (2) @kboyarinov prepared the fix for the failed CI tasks at include/oneapi/dpl/internal/common_config.h : see changes.
    Also this fix resolves a lot of errors in the tests compiled with g++ compiler.

Also we have the alternative approach at #1555

@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_tuple_get_const_rvalue_test_tmp branch from 15e1636 to cb340b8 Compare April 30, 2024 12:45
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_tuple_get_const_rvalue_test_tmp branch from 5f14762 to 2871802 Compare May 2, 2024 08:29
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_tuple_get_const_rvalue_test_tmp branch from 2871802 to e862fd3 Compare May 3, 2024 13:26
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM,
I think before merging (especially for inclusion with 2022.6.0), we should:

  1. Create an issue to add testing for include order which includes oneDPL headers before stdlib headers, since this changes all of our tests to include a stdlib header first ,as discussed offline.
  2. Get approval from another person.
  3. Run a full nightly CI with this branch to see the effect of the changes in common_config.h (that there are no unforeseen consequences) I believe that this is merely applying the same treatment for the missing TBB headers as we do for the mismatched tbb install above, so I think it should be OK.

UPD: the issue created: #1564

@danhoeflinger
Copy link
Contributor

Discussed offline, with another approval, it seems reasonable to merge and check nightly CI due to contention on runners rather than generating an extra full run. We just need to follow up to confirm we haven't caused trouble after the next run.

Copy link
Contributor

@timmiesmith timmiesmith left a comment

Choose a reason for hiding this comment

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

LGTM

@SergeyKopienko SergeyKopienko merged commit 589fbf2 into main May 6, 2024
20 checks passed
@SergeyKopienko SergeyKopienko deleted the dev/skopienko/fix_tuple_get_const_rvalue_test_tmp branch May 6, 2024 16:07
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.

5 participants