modelmat / sphinxcontrib-drawio

Sphinx extension for including draw.io files.
MIT License
39 stars 17 forks source link

Add drawio_disable_verbose_electron option #74

Closed jdillard closed 2 years ago

jdillard commented 2 years ago

I figured out you can add the Electron CLI --enable-logging option to get more information. For example this is the [stderr] output with logging on:

[stderr]
b'[3243:0511/224439.493686:ERROR:bus.cc(393)] Failed to connect to the bus: Failed to connect to socket /var/run/dbus/system_bus_socket: No such file or directory\n[3243:0511/224440.148733:ERROR:bus.cc(393)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")\n[3243:0511/224440.338936:ERROR:bus.cc(393)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")\n[3243:0511/224440.340409:ERROR:block_files.cc(466)] Failed to open /home/user/.config/draw.io/GPUCache/data_0\n[3243:0511/224440.505328:ERROR:gpu_process_host.cc(1003)] GPU process exited unexpectedly: exit_code=134\n[3243:0511/224440.505359:WARNING:gpu_process_host.cc(1317)] The GPU process has crashed 1 time(s)\n[3243:0511/224440.688088:ERROR:gpu_process_host.cc(1003)] GPU process exited unexpectedly: exit_code=134\n[3243:0511/224440.688113:WARNING:gpu_process_host.cc(1317)] The GPU process has crashed 2 time(s)\n'
[stdout]
b''
[returncode]
-7

If there aren't any errors, then the output of the extension is unchanged. This output was helpful in figuring out more information about the problems Electron was having, as outlined in this comment. I don't know if enable_logging should just always be on and not even an option, or the default should be True, but I left the default behavior as-is to be conservative.

Note: I'm working a potential similar "fix" that they implemented (so another PR in the works), that will allow the user to specify a list of strings to check for in the [stderr]. If there is a match, the exception passes so that in cases like this where nothing can be done to fix the issue, and the issue isn't really causing any problems, you can keep your CI from failing.

modelmat commented 2 years ago

Had a brief look at the code just then - if I understand correctly this only affects the output when the program errors?

If so, I see no reason for it to not be on by default. If you wish for it to be a config option something like enable_verbose_electron might be better. Definitely should be documented to be only when it errors if you do need a config option

jdillard commented 2 years ago

Correct, this only affects the output when the program errors. It also had negligible affects on performance.

I updated the name and defaulted it to True. I agree that it should be fine as True in most cases, and in the rare case that it might cause an issue, it can be turned off.

modelmat commented 2 years ago

You've typoed verbose as verbrose.

Thoughts on disable_verbose_electron? (Sorry if I just suggested the other name) We are defaulting to it being enabled so I was thinking it makes more sense naming it so the reason why you would adjust it.

jdillard commented 2 years ago

Doh, I fixed the typo. I also agree on changing to disable, I was thinking the same thing when I was making the change, but talked myself out of it since I originally was trying to copy how no_sandbox was implemented, but from first principles thinking disable makes the most sense. Do the changes look good now? I don't mind tweaking if they still need it.

modelmat commented 2 years ago

Would you like me to push a release?