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

Rename test binaries that collide with C++ standard library header names #1531

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

upsj
Copy link
Member

@upsj upsj commented Jan 17, 2024

This gets rid of all the CPATH issues we've been having from time to time by renaming all test binaries that match a C++ standard library header name.

Fixes #1530

@upsj upsj added the 1:ST:ready-for-review This PR is ready for review label Jan 17, 2024
@upsj upsj requested a review from a team January 17, 2024 10:42
@upsj upsj self-assigned this Jan 17, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:reference This is related to the reference module. labels Jan 17, 2024
@pratikvn
Copy link
Member

pratikvn commented Jan 17, 2024

I think it might be a bit annoying to maintain these lists ? Maybe we can prefix all test executables with a unique prefix/suffix like gkotest/gko ? So the executables would be named of the form gkotest_array/gko_array etc ?

@upsj
Copy link
Member Author

upsj commented Jan 17, 2024

I don't think it's that much effort, the list of C++ standard library headers is pretty short and doesn't change a lot, and we don't have many tests that overlap the list. That's something we can do during code review.

@upsj
Copy link
Member Author

upsj commented Jan 17, 2024

To go a bit more into detail why I dislike adding a prefix/suffix to all tests: I like being able to just build a single test binary via ninja rcm_gpu, the prefix/suffix makes that more annoying. The cause of this PR is a rather obscure issue that rarely impacts us, so I would like to make the workaround as unintrusive as possible.

@upsj upsj requested review from a team and removed request for a team January 17, 2024 10:53
@pratikvn
Copy link
Member

I see. That makes sense. Usually in my case, it is multiple targets that are relevant, so I end up doing a regex match with

ninja -t targets | grep ^foo | sed 's/:.*//' > my_targets

so a suffix/prefix does not affect this.

and then you can just run ninja $(cat my_targets) whenever necessary. This could also be something we could add to our CMake build step accepting a string from the user, but maybe that is over-engineering this.

@upsj
Copy link
Member Author

upsj commented Jan 17, 2024

What we could do there (and what I did before) is add meta-targets that compile certain parts of the library, e.g. a benchmarks target that compiles all benchmark binaries, a tests target that compiles all tests or stuff like that.

@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Jan 17, 2024
@upsj upsj merged commit 4558fb2 into develop Jan 17, 2024
13 of 15 checks passed
@upsj upsj deleted the fix_empty_cpath_builds branch January 17, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:reference This is related to the reference module. reg:build This is related to the build system. reg:testing This is related to testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ginkgo build crashes
5 participants