Closed tmeer closed 4 years ago
Note that the build fails (but does not error!) on CI, that is because the visualizer test cannot open a screen on the VM. I'll try to fix that.
You've clearly only looked at the Python tests on Ubuntu, the others are way more broken. Such is the hell of cross-platform C++ development.
Some notes:
image.display("Quantum Circuit");
, right? Can't you just save the image instead, and only display the image when the user requests this through an additional bool
in the visualize function? IMO handing an image file to the user is preferable over opening a window in 90% of the cases anyway.examples/cpp-standalone-example
in the repo. Running the usual cmake + build + run there must work without modification to its CMakeLists.txt, such that C++ programs and libraries depending on OpenQL don't have to change their CMakeLists every time something is added to OpenQL (from this point forward, anyway).#ifdef
s etc. for that. Note that the test suite should be smart enough to skip testing the visualizer as well in this case. Refer to the logic for disabling unitary decomposition as an example.visualize(program, layout)
is the only function that external code should call; that means that it and its dependencies are the only things that should be in the header file. #include "CImg.h"
, CircuitData
, and the other functions can just be at the top of visualizer.cc
file.Sorry if this comment comes off as a bit harsh, but in its current state this should not be merged into OpenQL. The above should be fixed first, and CI should be green without modifications to the pipelines, except maybe to add whatever (optional!) dependencies are needed to make the visualizer test(s) run on Mac.
If you need help with anything, let me know.
Your modification of the CI logic to use a newer compiler worries me. Not everyone has a newer compiler, or (if you're on some ancient TU centos server) has the ability to upgrade. In general, the solution to "CI fails because of the changes I made to OpenQL" is not to modify CI but to modify your changes to OpenQL, unless you have an extremely good reason. Every new dependency or environment modification is days upon days of work getting stuff to work again on everyone's machine.
That is a good point. I will modify the code to work with the default compiler.
OpenQL must be able to build, the tests must be able to run, and any OpenQL program that doesn't explicitly request a window be opened must run/compile properly in a headless server environment for various use cases. Build & test should never open a window or require user interaction. Correct me if I'm wrong, but you're just making an image and as a last step calling
image.display("Quantum Circuit");
, right? Can't you just save the image instead, and only display the image when the user requests this through an additionalbool
in the visualize function? IMO handing an image file to the user is preferable over opening a window in 90% of the cases anyway.To be honest, the test for the visualizer was more of temporary way of quickly checking additions I made to the visualizer. Having an option to save the image is something that is on the todo list. For now I will just remove the visualizer test program from git. I'll add the headless version later on, when I have implemented that image option.
Any required libraries must be linked in the toplevel CMakeLists in the "Configure, build, and link dependencies" section. The way you're doing it now (the way it used to work, to be fair), using OpenQL without Python will give link errors. See "Test / C++ standalone program" in CI and
examples/cpp-standalone-example
in the repo. Running the usual cmake + build + run there must work without modification to its CMakeLists.txt, such that C++ programs and libraries depending on OpenQL don't have to change their CMakeLists every time something is added to OpenQL (from this point forward, anyway).Ah, I see. I did not take using OpenQL without Python into account.
CImg.h has a pretty weird license, CeCILL-C for the header file itself and CeCILL for everything else to be specific. CeCILL-C is LGPL and therefore usable, but CeCILL is apparently a French law equivalent to GPL, which is not compatible with OpenQL's license. I think you're only using the header file and you're fine here, but please verify.
Will check.
Sorry if this comment comes off as a bit harsh, but in its current state this should not be merged into OpenQL. The above should be fixed first, and CI should be green without modifications to the pipelines, except maybe to add whatever (optional!) dependencies are needed to make the visualizer test(s) run on Mac.
No no, I really appreciate your comments. I will fix the issues you mentioned and make another pull request at a later moment.
Contains the integrated visualizer.
Note that the build fails (but does not error!) on CI, that is because the visualizer test cannot open a screen on the VM. I'll try to fix that.