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.

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

standardize graph output indentation with 2 spaces #8007

Closed

Conversation

evie404
Copy link
Contributor

@evie404 evie404 commented Apr 11, 2019

Currently, when outputting graphs from Bazel with bazel query --output=graph, the indentations for different types of lines are inconsistent:

  • node is indented with 2 spaces
  • edges and vertices are not indented
  • labels are indented with 1 space

Example using rules_go:

$ bazel query "deps(//go/platform:darwin)" --output=graph
digraph mygraph {
  node [shape=box];
"//go/platform:darwin"
"//go/platform:darwin" -> "//go/toolchain:darwin"
"//go/toolchain:darwin"
"//go/toolchain:darwin" -> "@bazel_tools//platforms:osx"
"@bazel_tools//platforms:osx"
"@bazel_tools//platforms:osx" -> "@bazel_tools//platforms:os"
"@bazel_tools//platforms:os"
}
Loading: 0 packages loaded

It would be nice for the indentations to be present (for edges and vertices), and consistent.

As for the indentation scheme, it seems inconclusive as to which type the language prefers:

  • the DOT Language Spec does not really specify tabs or spaces, but the examples used either contained no indentation (!) or one with 2 spaces
  • the official examples contain samples of no indentation, tabs, or 2 spaces, with tabs being most popular
  • the Wikipedia page uses tabs

I'm using 2 spaces to conform with skylark. This is the result from the same query with this PR:

$ /Users/ricky/workspace/bazel/bazel-bin/src/bazel query "deps(//go/platform:darwin)" --output=graph
Starting local Bazel server and connecting to it...
DEBUG: /private/var/tmp/_bazel_ricky/5c63e3be3c60ec773878bdf16f25adcc/external/bazel_toolchains/rules/version_check.bzl:45:9: 
Current running Bazel is not a release version and one was not defined explicitly in rbe_autoconfig target. Falling back to '0.23.0'
digraph mygraph {
  node [shape=box];
  "//go/platform:darwin"
  "//go/platform:darwin" -> "//go/toolchain:darwin"
  "//go/toolchain:darwin"
  "//go/toolchain:darwin" -> "@bazel_tools//platforms:osx"
  "@bazel_tools//platforms:osx"
  "@bazel_tools//platforms:osx" -> "@bazel_tools//platforms:os"
  "@bazel_tools//platforms:os"
}
Loading: 3 packages loaded

@aiuto aiuto requested a review from juliexxia April 15, 2019 15:38
@jin jin added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Apr 19, 2019
@gregestren
Copy link
Contributor

Nice catch! And thanks for such a thorough analysis.

I'd stick with two spaces, which fits closer to style guide recommendations although I acknowledge this isn't Starlark. I think of Bazel as a space-oriented system in general.

@evie404 evie404 force-pushed the rpai/graph_output_tab_indentation branch from 9dfdef4 to a1e69f1 Compare May 11, 2019 05:17
@evie404
Copy link
Contributor Author

evie404 commented May 11, 2019

@gregestren thanks a lot for the review. I rebased master due to merge conflicts and switched to 2 spaces in 9eb7b6d

let me know if there are other concerns.

@evie404 evie404 force-pushed the rpai/graph_output_tab_indentation branch from a1e69f1 to 9eb7b6d Compare May 11, 2019 05:19
@evie404 evie404 changed the title standardize graph output indentation with tabs standardize graph output indentation with 2 spaces May 11, 2019
@gregestren
Copy link
Contributor

One more quick request before we merge: could you update the PR description to mention that you're using spaces now and maybe throw out an "after" example at the end of what it the sample output looks like with your change?

@evie404
Copy link
Contributor Author

evie404 commented May 21, 2019

sure let me do that real quick

@evie404
Copy link
Contributor Author

evie404 commented May 21, 2019

@gregestren updated the PR description and posted after result

@gregestren
Copy link
Contributor

Thank you!

@gregestren
Copy link
Contributor

Just FYI I'm trying to merge this but running into a silly complication with the merge / export script. Need a bit of patience!

@evie404
Copy link
Contributor Author

evie404 commented May 23, 2019

no problem and no rush. let me know if there's any way i could help.

@gregestren
Copy link
Contributor

Update: still working through this. We still have a few query tests inside Google that haven't yet been exported to GitHub (I'm not aware of any reason why they couldn't be, so consider that a TODO). They embed expectations about the spacing which I think are just test brittleness, nothing wrong with what you're doing. But they highlight the possibility that people have tools/scripts that depend on the current formatting.

My current position is to still push this forward and if we find such cases then ask people to update their scripts. I don't believe this formatting is a guarantee of the Bazel user contract.

@bazel-io bazel-io closed this in 4ca768e Jun 3, 2019
@evie404 evie404 deleted the rpai/graph_output_tab_indentation branch June 3, 2019 20:34
@evie404
Copy link
Contributor Author

evie404 commented Jun 3, 2019

Thanks a lot!

irengrig pushed a commit to irengrig/bazel that referenced this pull request Jun 18, 2019
Currently, when outputting graphs from Bazel with `bazel query --output=graph`, the indentations for different types of lines are inconsistent:

- `node` is indented with 2 spaces
- edges and vertices are not indented
- labels are indented with 1 space

Example using [rules_go](https://github.com/bazelbuild/rules_go):

```
$ bazel query "deps(//gg/platform:darwin)" --output=graph
digraph mygraph {
  node [shape=box];
"//gg/platform:darwin"
"//gg/platform:darwin" -> "//gg/toolchain:darwin"
"//gg/toolchain:darwin"
"//gg/toolchain:darwin" -> "@bazel_tools//platforms:osx"
"@bazel_tools//platforms:osx"
"@bazel_tools//platforms:osx" -> "@bazel_tools//platforms:os"
"@bazel_tools//platforms:os"
}
Loading: 0 packages loaded
```

It would be nice for the indentations to be present (for edges and vertices), and consistent.

As for the indentation scheme, it seems inconclusive as to which type the language prefers:

- the [DOT Language Spec](https://www.graphviz.org/doc/info/lang.html) does not really specify tabs or spaces, but the examples used either contained no indentation (!) or one with 2 spaces
- the official [examples](https://www.graphviz.org/gallery/) contain samples of no indentation, tabs, or 2 spaces, with tabs being most popular
- the [Wikipedia page](https://en.wikipedia.org/wiki/DOT_(graph_description_language)) uses tabs

I'm using 2 spaces to conform with skylark. This is the result from the same query with this PR:

```plaintext
$ /Users/ricky/workspace/bazel/bazel-bin/src/bazel query "deps(//gg/platform:darwin)" --output=graph
Starting local Bazel server and connecting to it...
DEBUG: /private/var/tmp/_bazel_ricky/5c63e3be3c60ec773878bdf16f25adcc/external/bazel_toolchains/rules/version_check.bzl:45:9:
Current running Bazel is not a release version and one was not defined explicitly in rbe_autoconfig target. Falling back to '0.23.0'
digraph mygraph {
  node [shape=box];
  "//gg/platform:darwin"
  "//gg/platform:darwin" -> "//gg/toolchain:darwin"
  "//gg/toolchain:darwin"
  "//gg/toolchain:darwin" -> "@bazel_tools//platforms:osx"
  "@bazel_tools//platforms:osx"
  "@bazel_tools//platforms:osx" -> "@bazel_tools//platforms:os"
  "@bazel_tools//platforms:os"
}
Loading: 3 packages loaded
```

Closes bazelbuild#8007.

PiperOrigin-RevId: 251296052
irengrig pushed a commit to irengrig/bazel that referenced this pull request Jul 15, 2019
Currently, when outputting graphs from Bazel with `bazel query --output=graph`, the indentations for different types of lines are inconsistent:

- `node` is indented with 2 spaces
- edges and vertices are not indented
- labels are indented with 1 space

Example using [rules_go](https://github.com/bazelbuild/rules_go):

```
$ bazel query "deps(//gg/platform:darwin)" --output=graph
digraph mygraph {
  node [shape=box];
"//gg/platform:darwin"
"//gg/platform:darwin" -> "//gg/toolchain:darwin"
"//gg/toolchain:darwin"
"//gg/toolchain:darwin" -> "@bazel_tools//platforms:osx"
"@bazel_tools//platforms:osx"
"@bazel_tools//platforms:osx" -> "@bazel_tools//platforms:os"
"@bazel_tools//platforms:os"
}
Loading: 0 packages loaded
```

It would be nice for the indentations to be present (for edges and vertices), and consistent.

As for the indentation scheme, it seems inconclusive as to which type the language prefers:

- the [DOT Language Spec](https://www.graphviz.org/doc/info/lang.html) does not really specify tabs or spaces, but the examples used either contained no indentation (!) or one with 2 spaces
- the official [examples](https://www.graphviz.org/gallery/) contain samples of no indentation, tabs, or 2 spaces, with tabs being most popular
- the [Wikipedia page](https://en.wikipedia.org/wiki/DOT_(graph_description_language)) uses tabs

I'm using 2 spaces to conform with skylark. This is the result from the same query with this PR:

```plaintext
$ /Users/ricky/workspace/bazel/bazel-bin/src/bazel query "deps(//gg/platform:darwin)" --output=graph
Starting local Bazel server and connecting to it...
DEBUG: /private/var/tmp/_bazel_ricky/5c63e3be3c60ec773878bdf16f25adcc/external/bazel_toolchains/rules/version_check.bzl:45:9:
Current running Bazel is not a release version and one was not defined explicitly in rbe_autoconfig target. Falling back to '0.23.0'
digraph mygraph {
  node [shape=box];
  "//gg/platform:darwin"
  "//gg/platform:darwin" -> "//gg/toolchain:darwin"
  "//gg/toolchain:darwin"
  "//gg/toolchain:darwin" -> "@bazel_tools//platforms:osx"
  "@bazel_tools//platforms:osx"
  "@bazel_tools//platforms:osx" -> "@bazel_tools//platforms:os"
  "@bazel_tools//platforms:os"
}
Loading: 3 packages loaded
```

Closes bazelbuild#8007.

PiperOrigin-RevId: 251296052
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants