PUML not working on CI deployment (Teammates) due to lack of graphviz

[kaixin-hc]

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

See Teammates issue and reposense issue

Tell us about your environment

Live Deployed version

MarkBind version

3.1.1

What did you do? Describe the bug

After deployment, puml diagrams seem to be missing and have thrown an error.

Steps to reproduce

Unable to reproduce manually on local serve or on manual deployment on a forked branch

Expected behavior

.puml diagrams should display

Actual behavior

No response

Anything else?

Hypothesis: This is related to CI, and building .puml files during CI is the problem. CI may not have graphviz.

Observation: For MarkBind docs puml diagrams, we actually never use the tags within the website. Instead, we have saved the .puml as equivalent .pngs, and displayed the .png instead (see diagrams.md) Found this PR #917 explaining the reason is to avoid installing graphviz.

Graphviz related .puml errors: #1459, and the reposense: #1406 Replace images as PUML diagrams PR leading to #916 which was marked as fixed. Also consider reposense 1427 on the PUML diagrams not rendering

Suggestions? What should we do about this issue?

[tlylt]

Putting some details of the investigation that I have done here for reference:

• Graphviz does not seem to be required for sequence and activity diagrams, so it should be fine for anyone to refer to .puml files directly in CI (They might still need to ensure that Java is available for puml plugin to work). This is mentioned in the original puml PR, but was requested to change to "all diagrams", which is what we state in our UG at the moment. Perhaps we should consider specifying it as is (that some diagrams work out of the box)
  • 
• In GitHub Actions, if we are using Ubuntu latest (which is usually the case), Java is actually available (By default, Ubuntu 18.04 includes Open JDK 11), In general, it seems like windows-latest and macOS-latest do not have java installed by default and hence our current CI test should have reported a problem. (edit not sure about this)
• Given that we do not install Graphviz in our CI environment, in fact our existing functional tests (in test_site) are "broken". Some of the puml images generated are broken but because they are still generated and we only compare existence of the files, we thought that those files are generated correctly, when they are not (they are images containing similar error as the screenshot in the issue description).
• Given that Java is not available by default in macOS and windows (But somehow I think they are available, at least in GitHub Actions), there is a serious issue with how our call to run the plantuml executable does not error out. The related code is this. By changing the error output slightly, I was able to capture the error when ran with macOS, although it does not terminate the program:
  • 

[damithc]

Thanks for the investigation so far @tlylt
The ideal would be to support all PUML diagrams on GitHub Actions, if there is a way to do that. Otherwise we have to ask users to save a copy of the generate images locally, which goes against our claim of supporting PUML out of the box.

[tlylt]

Thanks for the investigation so far @tlylt The ideal would be to support all PUML diagrams on GitHub Actions, if there is a way to do that. Otherwise we have to ask users to save a copy of the generate images locally, which goes against our claim of supporting PUML out of the box.

Yes @damithc, I just tested it and it seems like as long as we do install java and Graphviz on CI (which is doable), then the images will be generated accordingly. I will drop a message in teammates' repo to ask if they wish to have this done (modifying the CI script) or hosting the images (the current changes by @kaixin-hc)

I think on our end we do need to put in a note in our deploy section, to inform users that if they are using puml diagrams, especially if they do continuous integration, they will have to ensure that their CI environment satisfy the required dependencies (Java and Graphviz).

[damithc]

Yes @damithc, I just tested it and it seems like as long as we do install java and Graphviz on CI (which is doable), then the images will be generated accordingly. I will drop a message in teammates' repo to ask if they wish to have this done (modifying the CI script) or hosting the images (the current changes by @kaixin-hc)

I think on our end we do need to put in a note in our deploy section, to inform users that if they are using puml diagrams, especially if they do continuous integration, they will have to ensure that their CI environment satisfy the required dependencies (Java and Graphviz).

Great. Thanks for the quick action @tlylt

[wkurniawan07]

Given that we do not install Graphviz in our CI environment, in fact our existing functional tests (in test_site) are "broken". Some of the puml images generated are broken but because they are still generated and we only compare existence of the files, we thought that those files are generated correctly, when they are not (they are images containing similar error as the screenshot in the issue description).

If I may add, the fact that this does not fail the CI build is itself a red flag.

[ang-zeyu]

Related to #1863.

In short, we've been (again presumably - I don't have context) prioritising usability of markbind serve over this.

@tlylt referring to #1245 (comment) as well, we can also aim to keep the current behaviour for markbind serve (show the puml error image), but fail hard for build. (for consistency with the createErrorNode function)

Given that we do not install Graphviz in our CI environment, in fact our existing functional tests (in test_site) are "broken". Some of the puml images generated are broken but because they are still generated and we only compare existence of the files, we thought that those files are generated correctly, when they are not (they are images containing similar error as the screenshot in the issue description).

Iirc this was discussed and acknowledged verbally (or maybe in some PR 👀) among us 2 years back. The rationale (not too big of an issue) is as we aim to test only the file paths we supply to the puml process.

Having the puml images error out would still generate error images, which is plenty sufficient for verifying said file paths. (we assume puml would do its things correctly, and leave testing that to the puml team)

In the case of graphviz errors, we assume the author has knowledge that it is required, which we've also documented quite clearly since the original PR by @alyip98.

https://markbind.org/userGuide/components/imagesAndDiagrams.html#diagrams

[kaixin-hc]

Based on the work done on this issue + the two original issues that flagged now being fixed, I think this issue can be closed now. If anyone else has any thoughts on this, let me know : )

[tlylt]

Based on the work done on this issue + the two original issues that flagged now being fixed, I think this issue can be closed now. If anyone else has any thoughts on this, let me know : )

Should be ok to close. The problem of not throwing an error when plantuml diagrams generation fails can be discussed in #1863