Skip to content
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

Merged
merged 11 commits into from
Jul 14, 2023
Merged

Add symbolator #1566

merged 11 commits into from
Jul 14, 2023

Conversation

zebreus
Copy link
Contributor

@zebreus zebreus commented Jun 8, 2023

This PR will add support for symbolator component diagrams.

resolves #1505

@ggrossetie
Copy link
Member

Woot 🎉
@zebreus Is it ready for review?

@zebreus zebreus marked this pull request as ready for review June 10, 2023 16:25
@zebreus
Copy link
Contributor Author

zebreus commented Jun 10, 2023

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.

@zebreus zebreus force-pushed the main branch 3 times, most recently from d619357 to 7ea2843 Compare June 10, 2023 17:08
@ggrossetie
Copy link
Member

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.

That's not reassuring 😨
I didn't see that... if the project is unmaintained, I don't think we should add it in the "main" distribution. I found a "popular" project named https://github.com/logisim-evolution/logisim-evolution. Maybe they would be interested in working/maintaining a diagram library to render HDL files as images?

I'm planning to work on #1423. That would allow third-party companion containers to register to the gateway server.

@zebreus
Copy link
Contributor Author

zebreus commented Jun 12, 2023

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.

@ggrossetie
Copy link
Member

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.

You can keep the test but it should check that the binary exists.

docs/antora.yml Outdated Show resolved Hide resolved
@ggrossetie
Copy link
Member

Maybe I was a bit to negative in my communication about symbolator. The custom version is mostly necessary to get it running with recent versions of python3. Symbolator itself is stable. The other changes are mostly small improvements like cli options to select a component or to get a diagram with a transparent background.

Got it 👍🏻

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).

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.

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.

👍🏻

@ggrossetie
Copy link
Member

Looks good, thanks!
I left a few comments that need to be resolved before we can merge.

@zebreus
Copy link
Contributor Author

zebreus commented Jun 21, 2023

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.

You can keep the test but it should check that the binary exists.

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.

@ggrossetie
Copy link
Member

If you want to I could add a few testcases somewhere else.

No I think it's OK.

@ggrossetie ggrossetie merged commit 482f7dc into yuzutech:main Jul 14, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Hardware Definition Language
2 participants