martinthomson / i-d-template

A template for IETF internet draft git repositories
Other
204 stars 180 forks source link

Support optional usage of mermaid-cli #416

Closed aj-stein-nist closed 7 months ago

aj-stein-nist commented 7 months ago

Hello, I am working on drafts with the IETF SCITT WG and we wanted to evaluate the use of Mermaid for at least some of our diagrams, not just aasvg. I noticed you had asked another WG contributor if they had considered Mermaid in https://github.com/martinthomson/i-d-template/pull/355#issuecomment-1336539483.

It would require the installation of necessary tools in the Docker image for this to work in GitHub Actions and you have expressed concerns about keeping the container image small, in this and other issues. I will open a PR but will you consider it inclusion given your comment?

OR13 commented 7 months ago

In theory it should be possible with:

npm i --save @mermaid-js/mermaid-cli;
git add package.json;
git commit -m "Add support for mermaid diagrams";

Then adding this markdown to the draft:

# mermaid

~~~mermaid
erDiagram
    CUSTOMER ||--o{ ORDER : places
    ORDER ||--|{ LINE-ITEM : contains
    CUSTOMER }|..|{ DELIVERY-ADDRESS : uses
~~~

In practice this does not work, because mermaid cli requires pupeteer, which cannot find chrome.

aj-stein-nist commented 7 months ago

In practice this does not work, because mermaid cli requires pupeteer, which cannot find chrome

I am drafting PR right now, it would need to be added to the Docker container upon which the GHA workflows run, and if that is merged and approved we can add the necessary variables to make this work.

martinthomson commented 7 months ago

So you are saying that npm i doesn't work?

For that then, I would recommend the mega image. Adding Chromium to the default image increases its size considerably:

action-mermaid   latest    ce4a0cba3c7d   11 seconds ago   846MB
action-base      latest    df8b7e7255ec   3 minutes ago    263MB
aj-stein-nist commented 7 months ago

So you are saying that npm i doesn't work?

For that then, I would recommend the mega image. Adding Chromium to the default image increases its size considerably ...

Thanks for the recommendation. Can I override the image used by GHA in the workflow? I'll read the action.yml and your documentation. Then I will follow up.

martinthomson commented 7 months ago

The way you can choose a different image is to refer to the @v1m action. That refers to a different image. See .../docker/math for the Dockerfile for that.

aj-stein-nist commented 7 months ago

OK so I would still need to install Chromium in that, right? I rebased that PR apologies for the delay in #417. It is now just the commit below targeting the docker/math/Dockerfile deps build for re-review.

https://github.com/aj-stein-nist/i-d-template/commit/7fc3237d16dbfac2ca2d3cf3cc2b665e60230221

aj-stein-nist commented 7 months ago

UPDATE: I read through the Makefiles and other code in the repo, I now realize this must be kramdown-rfc functionality having read its code, I will cross this out but assume Option 1 is not desirable short-term anyway.

~OK some last few questions, I am closer but not done done with this. The rest is specific to handling mermaid-cli/mmdc configuration at runtime. There are two approaches to this because I have to use the Chromium sandbox or create a docker container user and potentially break other users' repos (since there can be a lot of customization at this point).~

~1. I need to pass in --no-sandbox argument at runtime in the container through the GitHub Action so that mermaid properly uses Puppeteer as part of its routines. Since the container runs as root for now, I have to pass this in. Chromium will not support otherwise.~ ~2. I can add a related PR to define and default to running with a user when using the container, in GHA workflows or elsewhere.~

~I presume 2 is a long-term change and presumably undesirable for wide-scale use by IETFers, so I will presume I am moving forward with 1.~

~So re 1, I know I have to configure my repo accordingly but I cannot figure out how this projects Makefiles and tool combo can detect ```mermaid entries and run mmdc correctly? I need to figure out how to add arguments and point to a configuration file (it will not read it by default). Am I missing something important?~

Context: https://github.com/ietf-wg-scitt/draft-ietf-scitt-architecture/actions/runs/7116265158/job/19374293364

Relevant output:

*** [mermaid:] 
*** [mermaid:] Error: Failed to launch the browser process!
*** [mermaid:] [1206/145942.073366:ERROR:zygote_host_impl_linux.cc(100)] Running as root without --no-sandbox is not supported. See https://crbug.com/638180.
*** [mermaid:] 
*** [mermaid:] 
*** [mermaid:] TROUBLESHOOTING: https://pptr.dev/troubleshooting
*** [mermaid:] 
*** [mermaid:]     at Interface.onClose (file:///github/workspace/node_modules/@puppeteer/browsers/lib/esm/launch.js:253:24)
*** [mermaid:]     at Interface.emit (node:events:529:35)
*** [mermaid:]     at Interface.close (node:internal/readline/interface:534:10)
*** [mermaid:]     at Socket.onend (node:internal/readline/interface:260:10)
*** [mermaid:]     at Socket.emit (node:events:529:35)
*** [mermaid:]     at endReadableNT (node:internal/streams/readable:1368:12)
*** [mermaid:]     at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
*** [mermaid:] 
*** Can't clean SVG: undefined method `root_node' for nil:NilClass
*** [svgcheck:] ERROR: Unable to parse the XML document: /tmp/kramdown-rfc20231206-151-jsossx
*** could not create svg for "\nsequenceDiagram\n...
Error: Unable to parse the XML document: /dev/stdin
OR13 commented 7 months ago

yeah, this should def be in the larger image.

I would assume that if the larger image had chromium this would be all that is needed:

https://github.com/martinthomson/i-d-template/issues/416#issuecomment-1836866786

martinthomson commented 7 months ago

Give it a go. The large image was updated when @aj-stein-nist's pull request was merged.

aj-stein-nist commented 7 months ago

Sorry, I was away for a little bit. I did make the necessary changes but the emitted Mermaid diagram fails svgcheck validation. I will look more into this later but that is hardly an issue with this tooling, within the scope of this issue or others.

I will close this as implemented now unless people need further documentation in this repo or elsewhere, but it turns out I was just not reading what was present. So I am comfortable closing as-is now unless Martin or others want to speak up. Thanks again for a great tool.