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

feat: add tbb and testing to container #84

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vaeng
Copy link
Contributor

@vaeng vaeng commented May 14, 2024

According to the discussion on the forums, I added the tbb library to the container and wrote a small (very basic) test to check if it is working as intended.

I'd like to have the opinion of @ahans as well, as they suggested adding it if I remember correctly.

@vaeng vaeng requested a review from a team as a code owner May 14, 2024 13:53
@ahans
Copy link

ahans commented May 14, 2024

I'd like to have the opinion of @ahans as well, as they suggested adding it if I remember correctly.

I will have a look later this week!

Comment on lines +47 to +50
find_package(TBB)
if (${TBB_FOUND})
target_link_libraries(${exercise} PRIVATE TBB::tbb)
endif()
Copy link

Choose a reason for hiding this comment

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

I think it's appropriate to REQUIRE TBB here:

Suggested change
find_package(TBB)
if (${TBB_FOUND})
target_link_libraries(${exercise} PRIVATE TBB::tbb)
endif()
find_package(TBB REQUIRED)
target_link_libraries(${exercise} PRIVATE TBB::tbb)

We may want to edit the exercise over in cpp to also REQUIRE TBB now. In addition to the CMakeLists.txt, the documentation would also need an update then.

Dockerfile Outdated Show resolved Hide resolved
tests/example-tbb/CMakeLists.txt Outdated Show resolved Hide resolved
tests/example-tbb/example_tbb.cpp Outdated Show resolved Hide resolved
tests/example-tbb/example_tbb.cpp Outdated Show resolved Hide resolved
tests/example-tbb/example_tbb.cpp Outdated Show resolved Hide resolved
tests/example-tbb/example_tbb.cpp Show resolved Hide resolved
@vaeng vaeng requested a review from a team as a code owner November 21, 2024 09:09
@ErikSchierboom
Copy link
Member

Looks like CI is failing

@vaeng
Copy link
Contributor Author

vaeng commented Nov 21, 2024

Looks like CI is failing

Because I left some unused variables around. Now it is fine to merge.
I got your approval, but as you are not a guardian, the system is unhappy. @ErikSchierboom

@ErikSchierboom
Copy link
Member

@vaeng Let's get someone from the @exercism/guardians team to approve then

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