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.

Sign up for GitHub

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

Jump to bottom

Make graphviz font configurable #76794

Merged
merged 2 commits into from
Sep 16, 2020
Merged

Make graphviz font configurable #76794

merged 2 commits into from
Sep 16, 2020

Conversation

richkadel
Copy link
Contributor

Alternative to PR #76776.

To change the graphviz output to use an alternative fontname value,
add a command line option like: rustc --graphviz-font=monospace.

r? @ecstatic-morse

@richkadel
Make graphviz font configurable
5c29332 
Alternative to PR #rust-lang#76776.

To change the graphviz output to use an alternative `fontname` value,
add a command line option like: `rustc --graphviz-font=monospace`.
@richkadel richkadel mentioned this pull request Sep 16, 2020
Closed
@ecstatic-morse
Copy link
Contributor

I specifically want to use an environment variable as opposed to a flag for this so I can "set it and forget it" in my .profile. Defaulting to "Courier" is fine, since others may want to use VSCode extensions as you did.

I was going to say we should just fix this upstream: If vis.js only knows how to do layout for 4 fonts, maybe it should, uh, pick one of those? However, vis.js seems to have been abandoned. Oh well.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 16, 2020 •
edited
Loading

I'll just add a commit on top of this to switch to environment variables instead and push it to #76794. You don't have to implement all my weird requirements 😄.

@richkadel
Copy link
Contributor Author

I wish fixing it upstream was easy, but even if the project was still maintained, fixing this would involve updating all of the plugins that use it to the new version as well.

And, just because I was curious, I've been researching where these font limitations originate. The d3-graphviz library appears to depend on a WASM import of the native graphviz library. It looks like the limitations originate from graphviz dot's native rendering engine. Their FAQ speaks to some of the complexities of this kind of problem:

https://gitlab.com/graphviz/graphviz/-/blob/116c41100e2a3ddaa84e9b5586509befe12f2e98/doc/fontfaq.txt

@richkadel
Copy link
Contributor Author

richkadel commented Sep 16, 2020 •
edited
Loading

[corrected]
For your environment variable, would it be sufficient to:

export RUSTFLAGS="--graphviz-font=monospace"

@ecstatic-morse
Copy link
Contributor

No. I have to change RUSTFLAGS for unrelated reasons rather often, plus its tracked by x.py and will cause a full rebuild if I have to change it for some reason. Please, just let me have my workflow.

No gd and no cairopango =====
This is basically the original Graphviz without any external fonts.
It cannot render any raster formats, so it's mainly good for Postscript.
It relies on a few internal font tables

Presumably vis.js is in this bucket since it compiles dot to wasm and I don't know how you would link to system libraries. How hard would it be to preprocess fontfamily directives, though? Clearly vis.js knows what's in the "internal font tables"?

@richkadel
Copy link
Contributor Author

Sure, I'm not trying to break your workflow. Just exploring the options. Most of the compiler configs seem to be through flags, not as much through environment variables, but I can add support for an environment variable to this PR.

@ecstatic-morse
Copy link
Contributor

@bors delegate+ rollup

r=me whenever you're confident.

@bors
Copy link
Contributor

bors commented Sep 16, 2020

✌️ @richkadel can now approve this pull request

@richkadel
Added RUSTC_GRAPHVIZ_FONT environment variable
3875abe 
Overrides the debugging_opts.graphviz_font setting.
@richkadel
Copy link
Contributor Author

@bors r=ecstatic-morse rollup

@bors
Copy link
Contributor

bors commented Sep 16, 2020

📌 Commit 3875abe has been approved by ecstatic-morse

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 16, 2020
@tmandry tmandry mentioned this pull request Sep 16, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 16, 2020
@bors
Auto merge of rust-lang#76804 - tmandry:rollup-nwntt3q, r=tmandry
285fc7d 
Rollup of 16 pull requests

Successful merges:

 - rust-lang#75026 (Add array_windows fn)
 - rust-lang#76642 (Do not lint ignored private doc tests)
 - rust-lang#76719 (Change error message for ty param in const)
 - rust-lang#76721 (Use intra-doc links in `core::mem`)
 - rust-lang#76728 (Add a comment why `extern crate` is necessary for rustdoc)
 - rust-lang#76735 (Remove unnecessary `clone()`s in bootstrap)
 - rust-lang#76741 (Avoid printing dry run timings)
 - rust-lang#76747 (Add missing code examples in libcore)
 - rust-lang#76756 (fix a couple of stylistic clippy warnings)
 - rust-lang#76758 ([fuchsia] Propagate the userspace UTC clock)
 - rust-lang#76759 (Fix stabilization marker for future_readiness_fns)
 - rust-lang#76760 (don't lazily evaluate some trivial values for Option::None replacements (clippy::unnecessary_lazy_evaluations))
 - rust-lang#76764 (Update books)
 - rust-lang#76775 (Strip a single leading tab when rendering dataflow diffs)
 - rust-lang#76778 (Simplify iter fuse struct doc)
 - rust-lang#76794 (Make graphviz font configurable)

Failed merges:

r? `@ghost`
@bors bors merged commit 3bf66ae into rust-lang:master Sep 16, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecstatic-morse ecstatic-morse ecstatic-morse left review comments

Assignees
No one assigned
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@richkadel @ecstatic-morse @bors @rustbot