plasmodic / ecto

ecto is a dynamically configurable Directed Acyclic processing Graph (DAG) framework.
http://ecto.willowgarage.com/
BSD 3-Clause "New" or "Revised" License
97 stars 37 forks source link

Bugfix. Inconsistent behaviour in runtime #293

Closed drmateo closed 7 years ago

drmateo commented 7 years ago

This pull request refers to previous attempts #291 and #292.

stonier commented 7 years ago

+1 for bugfixing the return code on process(). What's the logic for dropping the plasm_loader dependency?

drmateo commented 7 years ago

Remove the plasm_loader dependency is not actually necessary because that change was just an attempt to resolve a problem in compilation time when you try to build the library tests using boost 1.62.

But I've detected the problem is situated in the TEST(SerialTest, Plasm) in the file named test/cpp/serialization.cpp .

My workaround solution (now I don't have time to find another better solution) is just avoid this test to be able to build and run the remaining tests. And as it is not necessary remove plasm_loader dependency, I do a rollback in the file test/scripts/CMakeLists.txt.

stonier commented 7 years ago

Two separate things here then. Each PR should cover a single problem or related set of problems.

The missing return bugfix gets a +1 from me to go in immediately. Can you make a PR covering that one exactly?

The latter workaround I'm not comfortable in just dropping from the build just because it doesn't compile on an as yet unsupported dependency. In my experience, these things never come back to be checked later if just hidden. I've fired up an issue in #294 where we can talk about it. @vrabaud - thoughts?

drmateo commented 7 years ago

The missing return bugfix gets a +1 from me to go in immediately. Can you make a PR covering that one exactly?

+1. I'm going to create a new PR just covering that problem

The latter workaround I'm not comfortable in just dropping from the build just because it doesn't compile on an as yet unsupported dependency. In my experience, these things never come back to be checked later if just hidden. I've fired up an issue in #294 where we can talk about it. @vrabaud - thoughts?

I'll continue handle this test problem in the issue #294 .

drmateo commented 7 years ago

The issue number related with the new PR (covering just the missing return problem) is #295

stonier commented 7 years ago

Closing this here then - @vrabaud if you tune in, we are moving discussion to issue #294.