-
-
Notifications
You must be signed in to change notification settings - Fork 225
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 symbolator #1566
Add symbolator #1566
Conversation
Woot 🎉 |
Now it is ready for review. My changes are mostly based on the existing code for ditaa. The upstream of symbolator is broken with newer python versions and unmaintained for 6 years. There was a updated fork but that one also seems abandoned for at least a year. VHDL support is also broken at that fork. I forked it again, merged all open PRs and added a few small features. The kroki docker build installs symbolator directly from the GitHub source, I also contacted the original author to add me as a maintainer on pypi so in the future it should be able to be installed from there. I packaged symbolator as AppImage and docker container these are probably the easiest ways to get a executable for testing. I also messaged the original author @kevinpt about publishing the fork on pypi so it can be installed with pip. I added a test to check that symbolator is kind of working, maybe that should be removed as it makes the server build depend on having a symbolator executable. |
d619357
to
7ea2843
Compare
That's not reassuring 😨 I'm planning to work on #1423. That would allow third-party companion containers to register to the gateway server. |
While logisim looks quite interesiting, its scope is different and a lot bigger than symbolator. Symbolators diagrams do not show connections between components or any kind of logic. They only shows the external interface of a single component. So symbolator is basically finished as it achieves everything it aims to achieve. As far as I am aware there is currently no other tool that could replace symbolator, although it probably would not be that hard to build one. Maybe I was a bit to negative in my communication about symbolator. The custom version is mostly neccessary to get it running with recent versions of python3. Symbolator itself is stable. The other changes are mostly small improvments like cli options to select a component or to get a diagram with a transparent background. Symbolator was also included in asciidoctor-diagram which was the reason why I used it in the first place. I think it should be in the main distribution of kroki to make transitions to asciidoctor.js and asciidoctor-web-pdf easier. Symbolator adds about 20MB to the image (from 1795MB to 1815MB). I can look into converting this to a companion container when service discovery is ready, but would love to have the changes upstreamed before, as I plan to use it in a project and would rather not host my own kroki server. |
You can keep the test but it should check that the binary exists. |
server/src/main/java/io/kroki/server/service/SymbolatorCommand.java
Outdated
Show resolved
Hide resolved
Got it 👍🏻
My main concerns were security and performance (i.e., if we encounter a security or performance issue and nobody can fix it). I thought about providing a way to dynamically disable a particular diagram library.
👍🏻 |
Looks good, thanks! |
I put that test along the service tests, but already removed it a few commits ago, as it did not really belong there (SymbolatorServiceTest.java). The smoke tests achieve basically the same thing, except for checking the output. If you want to I could add a few testcases somewhere else. |
No I think it's OK. |
This PR will add support for symbolator component diagrams.
resolves #1505