-
Notifications
You must be signed in to change notification settings - Fork 764
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
add tbb to travis-ci #210
add tbb to travis-ci #210
Conversation
hmm interestingly clang with release mode errors out with TBB: https://travis-ci.com/borglab/gtsam/jobs/274705966#L1506 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, but this will significantly increase CI time. I propose to make every Debug build be TBB=OFF, and just add one “special” build with TBB and Debug to make sure stuff compiles.
Only on clang on Linux. Very bizarre. I propose - if you agree - to make the changes to Travis.yml as I suggested which will also trigger a new Travis build. This could be a Travis fluke, as it seems completely orthogonal to the changes. |
I’ll make the changes, but it may not be a fluke. I’ll compile on my machine with clang and see if the output matches. Basically tbb has its own version of ‘concurrent_unordered_map’, in which ‘emplace’ has a very slight difference. But maybe ‘emplace’ is not available in the TBB version shipped with Ubuntu 16. |
We ran into this issue with clang + Ubuntu 16.04 + TBB 4.4 at work. The problem is that TBB only enables C++ 11 features such as A possible hack/workaround:
Could also ifdef on TBB version number if it's fixed in recent version of TBB. |
@chrisbeall interesting. I just tested clang with TBB 2020 on my machine and I did not get any errors. We should do ifdef based on |
|
Should I add the commit to this PR or make another one? The travis build won't succeed otherwise. |
This PR, as indeed otherwise we can't land it... |
Sweet we got it building. LGTM! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that it builds! But, two comments to this PR:
- what happens if we are not compiling with TBB? I think maybe emplace - which would exist then? - is not used?
- looking at this pattern, which is copy/pasted 4 times, it seems the right solution is to add an "emplace" to ConcurrentMap if it's missing.
Yep, you're right. Adding emplace to ConcurrentMap would be the cleaner solution. It would also resolve the first point, since ConcurrentMap is only used if TBB is found, otherwise FastMap is used, which will already have an emplace function. I'll add the change either tomorrow or later today. EDIT: I have some deadlines coming up, so I won't be able to tackle this issue until then. |
I know it has been a while since I have responded, but I cannot seem to resolve the issue in a smart way. Since I do not have a Ubuntu 16 machine to test with either, this is making it hard to resolve this Clang issue. I had thought overriding the |
(cherry picked from commit b68cd58)
Addresses #209 (comment)
Not completely sure if I added the TBB compilation where you guys wanted it, but I used my best judgement. Feel free to change this PR.
This change is