fix: handle OSError importing cairosvg to avoid build crash because of missing system dependencies

[Guts]

Closes: #6817

[squidfunk]

Thanks for the PR. However, we're not considering merging it.

The reason is that if we wouldn't throw the error, it might go silent in CI builds, with users wondering why social cards are not building. We're already experiencing high demand in support for setting up Cairo, and this would lead to even more issues why social cards are not building, as users might forget to install the dependencies, so the build will succeed. Thus, we cannot succeed the build if the dependencies are not available.

A better solution is to only enable the social cards plugin only in CI using an environment variable, as the CI environment variable is already defined in most CI build environments, e.g., GitHub Actions and GitLab:

plugins:
  - social:
      enabled: !ENV CI

This will disable generation of social cards locally (which makes no sense anyway, as you cannot post local links to Twitter or other social media platforms) and keep them when built within CI. We always need to keep the economics of maintaining the project in sight when considering PRs for merging. I hope for your understanding. Thank you.

[Guts]

I get your rationale.
However, there are 2 commits in my PR precisely because I wanted to separate :

1. the first prevents mkdocs from crashing (cf the issue) but let the actual plugin's behavior (exiting the build with an exception)
2. the second suggests avoiding the error and continuing the build; which will fail if the user has opted for strict mode, which is how the Mkdocs ecosystem works.

According to your argument, I can understand that the second point is debatable and why not assume that end users are not aware of strict mode and don't read the logs (I have the same experience on my own projects). But the former should be integrated to reduce support queries following a mkdocs build crash, shouldn't it?

For the record, I'm already using the environment variable to activate/deactivate the plugin, but I haven't figured out how to activate it only on a Linux environment or depending on the presence of Cairo dependencies.

[kamilkrzyskow]

I agree that the OSError should be fixed, since expected errors shouldn't hard crash with a Traceback during MkDocs config load.
I think, it's not even possible to monkey patch it with a hook, because it happens during load 🤔

However, I also agree that people typically do not run MkDocs in strict mode, so just issuing a warning visible in the GitHub Actions log (which people also typically don't check) would lead to more discussions / issues titled "Social Cards enabled but aren't generated", which is not what we want.

Similarly to example here, the lack of installed cairo should raise the PluginError:

mkdocs-material/material/plugins/social/plugin.py

Lines 78 to 83 in 23f1886

	# Check dependencies
	if "Image" not in globals():
	raise PluginError(
	"Required dependencies of \"social\" plugin not found. "
	"Install with: pip install \"mkdocs-material[imaging]\""
	)

Something like this:

try:
    from cairosvg import svg2png
except (ImportError, OSError) as err:
    CAIRO_ERROR = err
else:
    CAIRO_ERROR = None
    
try:
    from PIL import Image, ImageDraw, ImageFont
except ImportError as err:
    PIL_ERROR = err
else:
    PIL_ERROR = None    

and you'd change the conditional here to check for the global variable with a more targeted error message:

mkdocs-material/material/plugins/social/plugin.py

Line 79 in 23f1886

if "Image" not in globals():

EDIT: and later if the user doesn't want to let the social plugin crash the build, then they could monkey patch it as well with social_plugin.config.enabled = not bool(social_plugin.__class__.__module__.CAIRO_ERROR)

[squidfunk]

Okay! Let's fix the OSError then – could you remove the second commit or open a new PR?

[kamilkrzyskow]

I see that @Guts has reacted, but no change 🤔So was "you" in "could you remove the second commit or open a new PR?" directed at me specifically or both of us 😅
Will you adjust the PR @Guts or am I free to do it myself? I think I'm eligible to make changes with "Maintainers are allowed to edit this pull request. ", because I have push rights 🤔

[Guts]

Sorry, I did not have time past days and not much either in coming next.

So feel free to make the changes!

[squidfunk]

It was targeted towards @Guts, the OP of the PR, but I don't mind you making the changes, @kamilkrzyskow ☺️

[kamilkrzyskow]

I made the errors a bit more explicit:

ERROR   -  Required dependencies of "social" plugin not found.
Install with: pip install "mkdocs-material[imaging]"
No module named 'PIL'
No module named 'cairosvg'

ERROR   -  "cairosvg" Python module is installed, but it crashed with:
no library called "cairo-2" was found
no library called "cairo" was found
no library called "libcairo-2" was found
cannot load library 'libcairo.so.2': error 0x7e.  Additionally, ctypes.util.find_library() did not manage to locate a library called 'libcairo.so.2'
cannot load library 'libcairo.2.dylib': error 0x7e.  Additionally, ctypes.util.find_library() did not manage to locate a library called 'libcairo.2.dylib'
cannot load library 'libcairo-2.dll': error 0x7e.  Additionally, ctypes.util.find_library() did not manage to locate a library called 'libcairo-2.dll'
---
Did you install the external CairoSVG library?
Did you add the binaries path to your system PATH?
Did you restart the Terminal to assure the PATH was reloaded?

Perhaps the questions at the end might be a hit or miss depending on the cairosvg OSError, but I only had them with connection to not found library.

[squidfunk]

Thanks for your work! I'd say we move out the troubleshooting info to our documentation.

[kamilkrzyskow]

The current errors look like this:

ERROR   -  Required dependencies of "social" plugin not found:
           - ModuleNotFoundError("No module named 'PIL'")
           - ModuleNotFoundError("No module named 'cairosvg'")

           --> Install with: pip install "mkdocs-material[imaging]"

ERROR   -  "cairosvg" Python module is installed, but it crashed with:
           no library called "cairo-2" was found
           no library called "cairo" was found
           no library called "libcairo-2" was found
           cannot load library 'libcairo.so.2': error 0x7e.  Additionally, ctypes.util.find_library() did
           not manage to locate a library called 'libcairo.so.2'
           cannot load library 'libcairo.2.dylib': error 0x7e.  Additionally, ctypes.util.find_library()
           did not manage to locate a library called 'libcairo.2.dylib'
           cannot load library 'libcairo-2.dll': error 0x7e.  Additionally, ctypes.util.find_library() did
           not manage to locate a library called 'libcairo-2.dll'

           --> Check out the troubleshooting guide: https://t.ly/dlwQ7

The troubleshooting guide first and foremost recommends to restart the Terminal windows to reload PATH and stuff.
Then goes into a bit more detail, as best as I could write it out. I didn't check the macOS script yet (with GitHub Actions would be my only option). Perhaps I should provide some simple command instead of the script 🤔
In Windows Powershell there is

$env:Path -split ';'

and it will display a list, just like the one in the Python script, but without the caironames etc.

I'll change the state of the PR to draft, since I wait for feedback ✌️

[kamilkrzyskow]

Made a test repository and run the scripts on each system:
https://github.com/kamilkrzyskow/test-macos-actions/actions/runs/8103418921

Everything works as expected, however it kind of sucks I can't create a simple Linux script for easier troubleshooting.
Unless I would create it specifically for Ubuntu / GitHub Actions and tell other disctro users to deal with it on their own.

---

I also think I might've misunderstood and the troubleshooting guide should've been at the bottom of the page, not inside the cairo section? 🤔

Changes were addressed, PR still in draft state.

[kamilkrzyskow]

Thanks to Snippets the Markdown became more readable, and allowed for a bit longer Linux script.
I've focused on Ubuntu and Manjaro, as the other BSD or SunOS Linux variants aren't that common.
Moved the structure and adjusted the t.ly link in the scripts.

The updated scripts can be seen running here:
https://github.com/kamilkrzyskow/test-macos-actions/actions/runs/8159277013

EDIT: Sorry for the review request, found a bug in the Linux script. It doesn't print the output of gcc...

EDIT: aaand it works
https://github.com/kamilkrzyskow/test-macos-actions/actions/runs/8161417732

[kamilkrzyskow]

Did another force push to span out the text to 80 characters in some places,

EDIT: And another one 😅, added periods to all tasks in Windows guide. Also added an explicit point about applying the changes.

[Guts]

Impressive and really valuable work 👏!

Thanks @kamilkrzyskow!

[kamilkrzyskow]

Impressive and really valuable work 👏!

It would be impressive if I would do it at the beginning, not after stumbling over different solutions and force pushing all over the place. Also the value will be tested once people actually use the guide haha. Tough to my defence if the initial PR was merged, everything would be "OK", just unfinished compared to now, so I can confidently say my PRs aren't the worst 😄

@Guts adding to #6817 (comment) perhaps send one of your writers the guide and see if they manage to sort out Cairo on their system.

[kamilkrzyskow]

The last thing I'm unsure about, as this wasn't in the change request before, is that I did set CAPITAL-cased global variables, and from the rest of the code base it seems that global log variables are lowercase, so there is no distinction between global and local case?

[squidfunk]

LGTM! Thanks for putting the time in, great work!

One final remark when viewing the documentation though: I think we should not use Snippets, as the scripts are very long and that adds a lot of unnecessary noise to the documentation. This was my fault, recommending it but I also did not know the scripts will get so long. The scripts do not explain anything, and are more or less intended to be "just run" by the user helping with troubleshooting, so I would say we just link to the raw.githubusercontent versions of them, ideally directly triggering a download if that is possible.

[kamilkrzyskow]

The scripts do not explain anything, and are more or less intended to be "just run" by the user helping with troubleshooting

Yes, that was my intention from the start, as I believe adding a lot of comments / print statements, would just make it longer, and a non-techy person would still not read it before running it. There is only one print at the end, saying if it found the path.

so I would say we just link to the raw.githubusercontent versions of them, ideally directly triggering a download if that is possible.

I will look into a command to download the script and pipe them to Python. I've seen some PowerShell scripts do that, but without Python. I think the same should be possible with a curl command on Unix.

[squidfunk]

I will look into a command to download the script and pipe them to Python. I've seen some PowerShell scripts do that, but without Python. I think the same should be possible with a curl command on Unix.

Perfect. Also link to the script that is downloaded via curl, so users can inspect it. Anything else would be fishy.

[kamilkrzyskow]

Done and a test here: https://github.com/kamilkrzyskow/test-macos-actions/actions/runs/8227806730
Ultimately, I also changed the UPPERCASE variables to lowercase, as I stumbled upon this in the search plugin:

mkdocs-material/src/plugins/search/plugin.py

Lines 33 to 36 in c8bae9b

	try:
	import jieba
	except ImportError:
	jieba = None

Not entirely the same, but still consistent with the lowercase log, so all variables are lowercase in the codebase, I hope I didn't mess it up now, as the previous review accepted the uppercased variables...

[squidfunk]

LGTM. I need to find a time slot to be able to merge this through to Insiders, but since the social plugin is entirely different there, I expect it to take some time. Please bear with me.

[kamilkrzyskow]

I can also make a PR towards the Insiders branch, if you want to to have the dependency logic the same there, but since the plugins have branched out into two different versions, I don't think they have to merge the change at the same time.

[kamilkrzyskow]

I had a look at the source code in Insiders, and I see that the dependency check and related error was moved out of on_config to _generate which is invoked concurrently after on_page_markdown 😵💫
I'm not sure why the change was made, the comment claims Pillow to be the minimal requirement, but there are no safety checks done for cairosvg, so it's assumed it's always installed anyway.

I would move out the safety logic back to on_config like the community version of the plugin, as I don't see advantages to throwing an error only after the concurrency jobs are invoked 🤔
I won't follow-through, until I'm sure we're on the same page.

[squidfunk]

Yes, that's why I said I need a little more time to investigate. I think I put it there because of a better editing experience when crafting social card layouts. I'm quite sure I put it in on_config first and moved it out because of several oddities, but I can't remember entirely – sorry, too many changes happened since then and my comment is not clear enough (that's why I stress to write good comments explaining design decisions so often).

We need to test it thoroughly before making any change to it, i.e. check how the social plugin behaves in all of those cases (building, serving, cairo error, pillow not available). I would like to work on this myself. Again, please give me some time.

[vedranmiletic]

Thanks for this, social plugin now works again inside pipenv venv on FreeBSD.