Merge multiple HBFs into STF#384
Conversation
|
With #387 merged, the tests should soon turn green. |
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
2 times, most recently
from
5cc9266 to
a1f2687
Compare
|
Error while checking build/O2/o2 for a1f2687d08d4d0fea2fb861cc0b9700de405aa37: Full log here. |
|
@dberzano could you please disable also |
|
Yes, I have noticed the new error. I was wondering whether this was the correct approach... see #390. |
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
from
a1f2687 to
5b8609e
Compare
|
Given we have not changed the code which is crashing, we should probably open an issue about it (since it's probably broken since a while) and disable the test. |
|
Error while checking build/O2/o2-dev-fairroot for 5b8609e43cc70939bcd77f51cbcf05f372fdd964: Full log here. |
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
from
5b8609e to
a6ed118
Compare
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
from
a6ed118 to
5cd8428
Compare
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
3 times, most recently
from
1525199 to
0e9f90f
Compare
|
Error while checking build/O2/o2-dev-fairroot for 0e9f90ffc17edd74051989d9c593b4173621355a: Full log here. |
|
@matthiasrichter this is my (hopefully final) cleanup for this. As I mentioned to you earlier, I've introduced a new helper class PayloadMerger which can be used to merge payloads (doh) in a configurable manner. This is done via policies, rather than subclassing, because in principle one could think of them as three separate knobs whose implementations we can mix and match. |
|
Error while checking build/O2/o2 for 009c6b010b180b66f72a32bd86347f40a60bc04c: Full log here. |
|
Error while checking build/o2checkcode/o2-daq for 0e9f90ffc17edd74051989d9c593b4173621355a: Full log here. |
There was a problem hiding this comment.
I believe we prefer the C++11 variant: using = ...
There was a problem hiding this comment.
this test is not using the BOOST testing framework as other tests. For consistency we should do so. Moreover, the "asserts" will most likely be compiled away in any release build (which defined -DNDEBUG).
There was a problem hiding this comment.
The advantages of a framework are for instance that the program does not crash at the first failure and we get a precise list of passes/fails.
There was a problem hiding this comment.
I can (and will) move to BOOST test, however I disagree on what you say:
- Tests themselves should not be compiled with
-DNDEBUG, ever. If we do, we should fix the build rules. - As noted elsewhere we should have the option to have two copies for each test: one compiled against the optimised library, one compiled with the full debug mode library.
- We should probably have a decision about which unit test framework to use, and not simply rely on consistency. FairRoot uses gtest (which is slightly nicer to read, at least for me, and does not depend on boost, which is always good for compile time) and I've heard good things about catch. @Barthelemy / @dberzano was there any evaluation of the framework for tests?
@dberzano to comment, since this is really a tools issue.
There was a problem hiding this comment.
I was under the impression that this decision was taken since BOOST is used via the standard macro O2_GENERATE_TESTS. It is successfully deployed in various places. ( I cannot find a disagreement concerning the assert statement since you agree that then one needs to take extra precautions such as a simple #undef NDEBUG in the translation unit of the unit test. )
There was a problem hiding this comment.
There was no formal evaluation. I remember a discussion in CWG11 about it. We went for boost.test because any ways we depend on boost. Using gtest meant adding a dependency. As far as I remember, that was the main argument (?).
Pragmatically, at this stage, I would be in favour of continuing with boost.test just because all the tests are using it (also in the other O2 repos such as flp proto). If someone is motivated to make the formal evaluation though, let's do it. If we decide to change there would be then some work to modify the CMake macro, the recipes and the existing tests.
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
from
743763e to
60ca0d6
Compare
|
Error while checking build/o2checkcode/o2-daq for 60ca0d6ce9991d0d7d9cbeea27c6d7efb280e206: Full log here. |
There was a problem hiding this comment.
return ! (m.count(id) < 3) is slightly shorter
There was a problem hiding this comment.
we don't need the else here
There was a problem hiding this comment.
return (map.count(id) < this->mOrbitsPerTimeframe);
There was a problem hiding this comment.
according to conventions, there should be a brace { even round single line statements
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
from
60ca0d6 to
784712f
Compare
|
@sawenzel I think I've addressed all your remarks. On a side note, I wonder why boost.test decided to report |
|
Error while checking build/o2checkcode/o2-daq for 784712f1c8dbfe2a2f1d2e95ae892e5e4dde4a34: Full log here. |
|
@sawenzel any idea of what might be wrong? I do not see any error in the code checker part. |
|
@ktf: My bad ... the extraction of the source path from the compilations database failed (probably due to a wrong regular expression): This command should extract the actual path to the git sources ... probably the |
Utilities/DataFlow/CMakeLists.txt
Outdated
There was a problem hiding this comment.
to me this seems no longer seems necessary
There was a problem hiding this comment.
because, as far as I know, the Boost framework works independently of compile flags (advantage over asserts).
There was a problem hiding this comment.
You selected the wrong line in your comment.
That said, the -O0 is required to make sure I get the right line numbers when in lldb.
There was a problem hiding this comment.
I think it would be desirable to compile the unit tests the same way we compile the rest (see another comment). In case a unit test fails, a developer can recompile the project using a debug build in which case the line information should be correct.
There was a problem hiding this comment.
We should have both optimised and non optimised, as I've already said. I think it's up to WP3 to find and implement a solution for that. Tests are not only there to ensure bugs are not reintroduced, but also to simplify development and evolution of the code, which you cannot easily do if -O2 is used for them. Unless a better solution is provided for such a use case, this will stay as is.
There was a problem hiding this comment.
The canonical solution is to make one more PR build for Debug and then one gets both optimised and non optimised builds. The solution now is an awkward mixture.
|
No, I do not want -O0 in PR testing. I want -O0 when I develop my stuff, I
run tests in lldb all the time . Until we have a simple way to do that I
prefer to have -O0 as default.
…On Mon, Jun 19, 2017 at 10:11 AM sawenzel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Utilities/DataFlow/CMakeLists.txt
<#384 (comment)>:
> @@ -94,5 +97,8 @@ O2_GENERATE_TESTS(
O2_GENERATE_MAN(NAME TimeframeReaderDevice)
O2_GENERATE_MAN(NAME TimeframeWriterDevice)
+O2_GENERATE_MAN(NAME SubframeBuilderDevice)
target_compile_options(test_TimeframeParser PUBLIC -O0 -g)
The canonical solution is to make one more PR build for Debug and then one
gets both optimised and non optimised builds. The solution now is an
awkward mixture.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#384 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMnsGrtjiZ4I6-mG-hy99vwQADq64IFaks5sFizfgaJpZM4NfVyj>
.
|
|
@alibuild: I disagree on this, for the following reasons: |
matthiasrichter
left a comment
matthiasrichter
left a comment
There was a problem hiding this comment.
That looks great and elegant. I have a few comments between the lines.
There was a problem hiding this comment.
I've been trying to find a better name, because plain identity lead to a somewhat wrong association. Since it's a/the default PayloadExtracator, maybe identityPayloadExtractor or extractDetectorPayloadIdentity. And a comment should be added.
There was a problem hiding this comment.
How about fullPayloadExtractor?
There was a problem hiding this comment.
At least for the new files we should start adding the correct public link with http instead https.
There was a problem hiding this comment.
for the record: we agreed to postpone this for the moment because it would break the PR code check.
There was a problem hiding this comment.
shouldn't that be available from the FairMQMessage in the FairMQParts?
There was a problem hiding this comment.
Indeed... I think this is just an obsolete comment. Will remove.
There was a problem hiding this comment.
I would call this function either makeId or makeIdFromHeartbeatHeader
There was a problem hiding this comment.
how about return Merger::identity(...) or the respective function name if it is renamed
There was a problem hiding this comment.
Al right, we can argue that outgoing is a local variable and should not be used after the Send. The list will still contain the individual messages but they will have empty unique_ptr objects. Like it is now we do not need to clean because the local variable is destroyed anyhow.
There was a problem hiding this comment.
Ok. I will remove the clean and the comment.
There was a problem hiding this comment.
I have started to use AliceO2 instead of man at this point to mark the AliceO2 related man pages.
Utilities/DataFlow/CMakeLists.txt
Outdated
There was a problem hiding this comment.
I'm also confused about hardwired compiler options. The flags are just appended and thus override any other optimization settings. The CI never has the chance to build the tests with some other setting. Even if this is ok for the tests, it's a bad example to have this in the cmake setup. It somehow undermines the idea that we globally take care of the cmake settings and dependencies.
There was a problem hiding this comment.
I surrender to the crowd and will remove them... Democracy simply does not work... ;-)
There was a problem hiding this comment.
Lets get rid of the 'self triggered' mode and the corresponding option. It doesn't do anything.
There was a problem hiding this comment.
lets get rid of this option, this was a simple mockup in the beginning of the development
This is now done by introducing a new generic class PayloadMerger which can be used to merge together multiple payloads into a given buffer, which can then be sent via O2. The PayloadMerger class can be configured w.r.t.: - Given a Payload, determine is the unique identifier for the class of equivalence it belongs to. - Given an id, determine wether or not the class of equivalence it represents is complete. - Given a payload, extract the parts you are interested from it.
ktf
force-pushed
the
dev-merge-hbf-into-stf
branch
from
784712f to
d0b7983
Compare
|
@matthiasrichter @sawenzel I think I addressed all the comments you had. Can you verify and eventually sign off? |
There was a problem hiding this comment.
for the record: we agreed to postpone this for the moment because it would break the PR code check.
6 participants
From your favourite coding duo, @matthiasrichter & @ktf