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

lfd binary for generating diagrams from the command line #1713

Merged
merged 21 commits into from
Jun 21, 2023
Merged

lfd binary for generating diagrams from the command line #1713

merged 21 commits into from
Jun 21, 2023

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Apr 20, 2023

This PR adds a new command line tool lfd. This tool generates an SVG from a given LF program. Currently, the diagram generation as well as the output format and file name cannot be configured. We can extend lfd with such options in subsequent PRs.

This PR also adds a new unit test that smoke tests the diagram synthesis. While checking the concrete diagram output is challenging, this at least tells us if there are any unexpected errors occurring during the diagram generation (like #1833).

The standalone diagram generation via KlighD also requires pulling in SWT as a dependency. SWT is platform specific and, in consequence, we now also need to create platform specific artifacts. This PR also adjusts the nightly release CI job to produce artifacts for Windows, Linux and MacOS for both x86_64 and aarch64 architectures (Windows only uses x86_64 as there is no aarch64 support for Windows in SWT).

Closes #897
Closes #802

@cmnrd cmnrd marked this pull request as draft April 20, 2023 12:16
@lhstrh
Copy link
Member

lhstrh commented Apr 27, 2023

@soerendomroes, would you be able to comment on this?

@soerendomroes
Copy link
Collaborator

@soerendomroes, would you be able to comment on this?

Not really, but I can try checking it out later today.

@soerendomroes
Copy link
Collaborator

It would be nice if someone could add an example JSON file or string to test this. Where can I find one? @cmnrd

@soerendomroes
Copy link
Collaborator

Bad type on operand stack
Exception Details:
  Location:
    de/cau/cs/kieler/klighd/piccolo/export/BitmapOffscreenRenderer.doRender(Lde/cau/cs/kieler/klighd/ViewContext;Ljava/io/OutputStream;Lorg/eclipse/elk/graph/properties/IPropertyHolder;)Lorg/eclipse/core/runtime/IStatus; @84: invokespecial
  Reason:
    Type 'org/eclipse/swt/widgets/Shell' (current frame, stack[2]) is not assignable to 'org/eclipse/swt/widgets/Composite'
  Current Frame:
    bci: @84
    flags: { }
    locals: { 'de/cau/cs/kieler/klighd/piccolo/export/BitmapOffscreenRenderer', 'de/cau/cs/kieler/klighd/ViewContext', 'java/io/OutputStream', 'org/eclipse/elk/graph/properties/IPropertyHolder', integer, 'org/eclipse/swt/widgets/Shell' }
    stack: { uninitialized 47, uninitialized 47, 'org/eclipse/swt/widgets/Shell', integer, 'java/awt/Color' }
  Bytecode:
    0000000: 2dc6 0015 2db2 004c b900 5002 00c0 0056
    0000010: b600 58a7 0011 b200 4cb9 005c 0100 c000
    0000020: 56b6 0058 3604 bb00 6059 b700 623a 05bb
    0000030: 0063 5919 0503 2dc6 0012 2db2 0065 b900
    0000040: 5002 00c0 006a a700 0eb2 0065 b900 5c01
    0000050: 00c0 006a b700 6c3a 062a 2b19 06b6 006f
    0000060: 2db6 0073 57a7 0014 3a07 bb00 2f59 0712
    0000070: 3112 7719 07b7 0033 b02d c600 122d b200
    0000080: 79b9 0050 0200 c000 7ca7 0005 127e 3a07
    0000090: bb00 8059 b700 8219 06bb 0083 592b 1907
    00000a0: 2cb7 0085 1504 b600 88b6 008c b600 9057
    00000b0: a700 143a 07bb 002f 5907 1231 1294 1907
    00000c0: b700 33b0 1905 b600 96b2 0099 b0       
  Exception Handler Table:
    bci [89, 101] => handler: 104
    bci [121, 176] => handler: 179
  Stackmap Table:
    same_frame(@22)
    same_locals_1_stack_item_frame(@36,Integer)
    full_frame(@73,{Object[#1],Object[#167],Object[#169],Object[#81],Integer,Object[#96]},{Uninitialized[#47],Uninitialized[#47],Object[#96],Integer})
    full_frame(@84,{Object[#1],Object[#167],Object[#169],Object[#81],Integer,Object[#96]},{Uninitialized[#47],Uninitialized[#47],Object[#96],Integer,Object[#106]})
    full_frame(@104,{Object[#1],Object[#167],Object[#169],Object[#81],Integer,Object[#96],Object[#99]},{Object[#157]})
    same_frame(@121)
    same_frame(@140)
    same_locals_1_stack_item_frame(@142,Object[#124])
    same_locals_1_stack_item_frame(@179,Object[#157])
    same_frame(@196)

Our SWT-Mock is the problem. In the real SWT Shell extends Decorations extend Canvas extends Composite, which is not the case for our mock. This error seems to occur during the creation of the BitmapOffscreenRenderer in the piccolo package. Therefore, you could either find a way to not load this class or wait until Klighd and the SWT-Mock are published again.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Apr 28, 2023

@soerendomroes Thanks! I could reproduce your finding when running lfd in a debug session from IntelliJ. Do you know if there is a way to get this output when running from the command line (i.e. directly execute the jar)? I have tried to manually strip the class from the jar, but this alone did not help, and I suspect there might be more errors.

Unfortunately, I didn't find a simple way to strip a class from the jar in gradle.

Is there an expected time horizon for publishing a new version of kligkd (and SWT-Mock)?

@soerendomroes
Copy link
Collaborator

Do you know if there is a way to get this output when running from the command line (i.e. directly execute the jar)?

It is written to the Klighd log Klighd.log(new Status(IStatus.ERROR, Klighd.PLUGIN_ID, STARTUP_HOOK_ERROR_MSG, t));

This will call the handle(Status) method of the registered StatusManager (or per default the Eclipse status manager). I suppose you could register it an start-up similar to this:

Klighd.setStatusManager((status, style) -> {
    // TODO call something to handle the status
});

See https://github.com/kieler/KLighD/blob/013d6687ead7fee7041eff36f98e02375e5b2b56/plugins/de.cau.cs.kieler.klighd/src/de/cau/cs/kieler/klighd/KlighdPlugin.java#L71 for an example.

Is there an expected time horizon for publishing a new version of kligkd (and SWT-Mock)?

@NiklasRentzCAU can you answer this?

@NiklasRentzCAU
Copy link

Is there an expected time horizon for publishing a new version of kligkd (and SWT-Mock)?

This has shifted to the top of my priority stack and I will start to look at implementing the Maven stuff as I had planned and start the process necessary to publish on official Maven repositories this week. If everything goes as planned we might have a new release of KLighD and the Mock as P2 Update sites and official Maven artifacts in the following weeks.

@NiklasRentzCAU
Copy link

I am just looking at this issue again and notice that releasing KLighD with the SWT mock will not solve your problem here. The SVG export as you currently use it will not work: You solution tries to utilize the BitmapOffscreenRenderer to render the diagram, which does use SWT (the real, platform-dependent SWT, not our mock). The mock will only be required when using the language server, but not for this use case.

What you need to use the offscreen renderer is to make sure to pull the real platform-dependent SWT artifacts for this command line tool and build three different versions, one for Windows/MacOSX/Linux each. The mock is only there to make the language server more leight-weight and make it platform-independent (which is exactly why we want to release that now), the Piccolo exporter, however, uses SWT and will need the real artifacts and therefore a more complex build.

@soerendomroes
Copy link
Collaborator

BitmapOffscreenRenderer will not be used, just freehep.

@cmnrd
Copy link
Collaborator Author

cmnrd commented May 10, 2023

BitmapOffscreenRenderer will not be used, just freehep.

@soerendomroes Does this mean we can export diagrams without adding the real SWT as a dependency?

Since it is possible to export diagrams when using the language server without requiring the real SWT, I assume this should also be possible in a standalone environment.

@soerendomroes
Copy link
Collaborator

No, I am wrong, you need the real SWT for freehep export. If you use a sprotty exporter it should however be possible to build one, but I don't know how.

@NiklasRentzCAU
Copy link

Technically possible, but there is no API for that, especially not from KLighD directly. For an "easy" way you will need to use the real SWT for this currently.

@cmnrd
Copy link
Collaborator Author

cmnrd commented May 10, 2023

I managed to pull in SWT with gradle (I think), but I still get the same error. Do I have to disable/exclude the SWT Mock explicitly? If so, now?

@cmnrd
Copy link
Collaborator Author

cmnrd commented May 10, 2023

Ok, I found a way to exclude the SWT Mock. I also had to add freehep-graphicsio to the dependencies (apparently this is not included automatically by piccolo.freehep). Now I don't see any exceptions anymore, but still I get the message that no offscreen renderer can be found by klighd. Any ideas why?

@NiklasRentzCAU
Copy link

I cannot fully pinpoint the issue here, but have a few pointers to make your solution work and to find the issue:

First of all, you should not globally exclude the SWT mock from your build, as that will also cause your language server to exclude the mock. The LS, however, definitely should use the mock to keep that platform-independent

The unofficial release of the Maven artifacts of KLighD that you use registers the offscreen renderer via service loader and not directly from the standalone setup that you call. It should be registered regardless if your jar/classpath has everything it needs, but you could try the following to fix/find the issue:

  • Call the KLighdDataManager to make sure the static loading of the extensions (including the offscreen renderers) took place. For that, call KlighdDataManager.getInstance() and assign it to some variable before trying to export. I assume this won't fix it though
  • Make sure that the PiccoloSetup and its registration for the service loader are in your classpath. If these are included, the registration should work.
  • Also make sure that if there are multiple files called https://github.com/kieler/KLighD/blob/nre/lsp-only-maven/plugins/de.cau.cs.kieler.klighd.piccolo/META-INF/services/de.cau.cs.kieler.klighd.IKlighdStartupHook that you include in your execution, that they are merged together into a single file with all of these contents, in particular the file should include one line with de.cau.cs.kieler.klighd.piccolo.export.PiccoloSetup.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 16, 2023

Today I recreated the changes after our gradle refactoring and spent quite some time debugging and stepping through the plugin registration process. It turned out that there was a class not found exception while registering the SVG renderer, which was masked by an empty catch all statement in klighd. The problem was easily fixed by adding freehep-graphicsio-svg to the dependencies.

Next I would like to add the diagram synthesis to our tests. For this I would like to detect if an error occured during the diagram generation. Currently, if there is an exception triggerd during the synthesis, there is a backtrace printed and the generated svg also shows an error message. However, there is no indication of failure when rendering the svg. Is there a way to detect that there was an exception during the diagram generation @a-sr, @NiklasRentzCAU? I assume that there is a catch statement somewhere that just catches all exceptions that occurred during the generation.

@NiklasRentzCAU
Copy link

You are correct, there are multiple catch statements logging the errors that may occur in different occasions to prevent the entire application from shutting down. For example, you can see that the ViewContext catches an exception and logs that with Klighd.log(...). You can intercept these messages by setting an own IKlighdStatusManager and handling the errors the way you want to.

@cmnrd cmnrd marked this pull request as ready for review June 19, 2023 16:36
@cmnrd cmnrd changed the title Add lfd binary for generating diagrams from the command line lfd binary for generating diagrams from the command line Jun 19, 2023
Comment on lines -42 to +50
return null;
return reportError(message);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am definitely more comfortable with these not being no-ops. But was there a reason why they used to be no-ops? For example, with this change, if someone tries to view a diagram when the model is invalid, will this cause problems, such as duplicated error messages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't know. Probably @a-sr can provide more context. I added this forwarding of messages via klighd's logger so that we get proper error reporting in lfd and can also intercept errors when testing. I don't know what klighd's logger does when we use this from Eclipse or the LSP. The default behavior of the logger is to print to stdout. If this is the case also for Epoch and the LSP, then some additional output in the log should not be problematic. We can try this out, and if we see duplicated messages we can always register a status handler that just discards the messages.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 20, 2023

Since our artifacts now include platform specific code, I adjusted the distribution logic to append the OS name. So that we get something like lf-cli-0.4.1-SNAPSHOT-Linux.tar.gz. I am not sure yet how we best handle the nightly build. We will need to provide 3 artifacts. @lhstrh @petervdonovan What do you think would be easier with respect to the CI setup?

I made some changes today that allow to specify the target OS and also the target architecture (x86_64 or aarch64). This should allow us to easily build all the artifacts in CI. I will later also update the build job.

@cmnrd cmnrd enabled auto-merge June 20, 2023 18:55
@cmnrd cmnrd added this pull request to the merge queue Jun 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 20, 2023
@cmnrd cmnrd added this pull request to the merge queue Jun 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 21, 2023
@cmnrd cmnrd added this pull request to the merge queue Jun 21, 2023
@cmnrd cmnrd removed this pull request from the merge queue due to a manual request Jun 21, 2023
@cmnrd cmnrd merged commit ce052df into master Jun 21, 2023
@cmnrd cmnrd deleted the lfd branch June 21, 2023 09:49
@lhstrh lhstrh added the feature New feature label Aug 28, 2023
@lhstrh lhstrh changed the title lfd binary for generating diagrams from the command line lfd binary for generating diagrams from the command line Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a separate lfd CLI tool for generating diagrams from LF programs Diagram synthesis should be tested
6 participants