-
Notifications
You must be signed in to change notification settings - Fork 56
Make the viewer binary a multi-purpose binary #136
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
Conversation
Change the start-up code so that the viewer binary can run in the following 4 modes: 1. viewer: open a windows 2. command: take a string as Python code 3. terminal: start text-based iPython prompt 4. pytest: run the Python test cases In this way, the viewer binary can be used to run the Python test cases. A new target run_viewer_pytest is added to the make file. CI (Ubuntu, macos, and Windows) uses the new target to run unit tests.
| run: | | ||
| cmake --build ${{ github.workspace }}/build ` | ||
| --config ${{ matrix.cmake_build_type }} ` | ||
| --target run_viewer_pytest |
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.
Unit testing on Windows is now running, but the return code is not checked yet.
There is a minor error that can be fixed in the future when we change to respect the return code.
|
|
||
| .PHONY: run_viewer_pytest | ||
| run_viewer_pytest: viewer | ||
| cmake --build $(BUILD_PATH) --target run_viewer_pytest VERBOSE=1 |
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.
Use cmake target. This should be more robust. Other make file targets should change to be like this in the future.
| // Initialize the Python interpreter. | ||
| modmesh::python::Interpreter::instance() | ||
| .initialize() | ||
| .setup_modmesh_path(); |
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.
Move the setup code to distinct functions.
|
|
||
| Interpreter & Interpreter::setup_process(int argc, char ** argv_in) | ||
| { | ||
| std::vector<std::string> argv; |
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.
This is a prototype implementation. Better organization should be done in the future.
| }, | ||
| py::arg("filename")) | ||
| .def( | ||
| "app", |
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.
Change it to be a function so that it always get the singleton from C++.
| self.exited_status = None | ||
| self.exited_message = None | ||
|
|
||
| def exit(self, status=0, message=None): |
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.
Override to avoid calling sys.exit.
| return pytest.main(['-v', '-x', mmpath]) | ||
|
|
||
|
|
||
| def setup_process(argv): |
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.
setup_process is probably not a good name because the role is the main function. But let's use it for now.
|
there is a bug, even if I clean up the code in the interpreter and run it, the graph result is still there. |
|
Actually, I think improving the interpreter could be a good project. probably can open another issue. like and the syntax highlight or something. |
Look forward to a PR. This PR is merged and it should be easier for you to work on it. |
Address #103
Change the start-up code so that the viewer binary can run in the following 4 modes:
In this way, the viewer binary can be used to run the Python test cases. A new target run_viewer_pytest is added to the make file. CI (Ubuntu, macos, and Windows) uses the new target to run unit tests.