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

Do not require access to dependencies that aren't being documented #93

Open
shs96c opened this issue Feb 26, 2021 · 11 comments
Open

Do not require access to dependencies that aren't being documented #93

shs96c opened this issue Feb 26, 2021 · 11 comments

Comments

@shs96c
Copy link

shs96c commented Feb 26, 2021

It's not always practical to add a dependency on the bzl files that come from third party dependencies. stardoc should allow documentation to be generated without introspecting deeply into dependencies.

As an example, I use MavenInfo from rules_jvm_external in my own rules for building java modules. In order to generate docs using stardoc, I must somehow also include source files from rules_jvm_external

shs96c added a commit to shs96c/rules_jvm_external that referenced this issue Feb 26, 2021
This allows anyone who depends on `rules_jvm_external` and imports
"something" from these rules into their own rules to be able to
generate stardoc of their own functions.

I've filed bazelbuild/stardoc#93 to track
this issue. Once it's fixed, we can close this.
shs96c added a commit to shs96c/rules_jvm_external that referenced this issue Feb 27, 2021
This allows anyone who depends on `rules_jvm_external` and imports
"something" from these rules into their own rules to be able to
generate stardoc of their own functions.

I've filed bazelbuild/stardoc#93 to track
this issue. Once it's fixed, we can close this.
@burkpojken
Copy link

I agree with that it would be very good to not have to depend on visibility of third party bzl files that you do not want to generated documentation for.
We see the same problem when are using skylib features in our bzl files for which we want to generate documentation.
Example we have our custom config settings in config.bzl
With content:
load(
"@bazel_skylib//rules:common_settings.bzl",
"bool_flag",
"string_flag",
)

def our_rule(
......
)
We want to generate documentation for our_rule()
Since the visibility of in the latest version of @bazel_skylib//rules:common_settings.bzl has been changed to
exports_files(
glob(["*.bzl"]),
visibility = ["//:subpackages"],
)
We cannot use stardoc anymore with the latest version of skylib. It does not feel right to request changes in skylib because of this.

@brandjon
Copy link
Member

brandjon commented Mar 4, 2021

See #69, this should be addressed when we rewrite the doc extraction to grab real objects from the loading phase instead of mocking it in a separate process.

@brandjon
Copy link
Member

brandjon commented Mar 4, 2021

(P4ing this one as opposed to marking it a dup, but the underlying rewrite is actually a P1 FR that we're aiming to work on this quarter and next.)

shs96c added a commit to shs96c/rules_jvm_external that referenced this issue Mar 11, 2021
This allows anyone who depends on `rules_jvm_external` and imports
"something" from these rules into their own rules to be able to
generate stardoc of their own functions.

I've filed bazelbuild/stardoc#93 to track
this issue. Once it's fixed, we can close this.
shs96c added a commit to bazel-contrib/rules_jvm_external that referenced this issue Mar 11, 2021
This allows anyone who depends on `rules_jvm_external` and imports
"something" from these rules into their own rules to be able to
generate stardoc of their own functions.

I've filed bazelbuild/stardoc#93 to track
this issue. Once it's fixed, we can close this.
@guw
Copy link

guw commented Apr 3, 2022

I also started to investigate Stardoc for us and quickly ran into this.

Currently unable to build because rules_pkg also has their .bzl files defined with limited visibility.

guw added a commit to guw/rules_pkg that referenced this issue Apr 3, 2022
haikalpribadi pushed a commit to typedb/typedb that referenced this issue Sep 20, 2022
## What is the goal of this PR?

Fix the issues in tar.gz assembly that were caused by #6642.

## What are the changes implemented in this PR?

PR #6642 has changed the upstream repository of `rules_docker` to be
from `bazelbuild`. Previously, the upstream repository was the Vaticle's
fork of `rules_docker`. That fork contained a patch that fixed a bug in
`rules_pkg` that stopped `pkg_tar` rule from working with long paths
(the bug caused targz assembly of TypeDB to be incorrect) .
Incidentally, an identical patch was applied in Vaticle's
`bazel-distribution`. By loading, `rules_docker` from
`vaticle_dependencies` before `bazel-distribution`, we ended up using
the unpatched `rules_pkg` in the assembly targets that were depending on
`rules_pkg` (we can't have multiple versions of a workspace dependency
at the same time). This caused the targz assembly of TypeDB to contain
invalid paths.

The solution in this PR simply swaps the order of loading, so that
`typedb` ends up depending on the patched `rules_pkg` that is provided
by `bazel-distribution`.

The ideal solution for this problem would be to bump the `rules_pkg`
dependency in `bazel-distribution`, however, the newer versions of
`rules_pkg` changed their subpackage visibility modifiers. More
precisely, in the newer versions, `pkg_tar` depends on private
subpackages. Unfortunately, `stardoc`, the tool that we use to generate
documentation, does not work with rules that depend on rules that are
invisible to our project
(bazelbuild/stardoc#93,
bazelbuild/stardoc#128). Therefore, in order
to continue using `stardoc`, we must continue using the older version of
`rules_pkg`.

Co-authored-by: Lukas Slezevicius <[email protected]>
Co-authored-by: Alex Walker <[email protected]>
flyingsilverfin pushed a commit to typedb/typedb that referenced this issue Nov 3, 2022
## What is the goal of this PR?

Fix the issues in tar.gz assembly that were caused by #6642.

## What are the changes implemented in this PR?

PR #6642 has changed the upstream repository of `rules_docker` to be
from `bazelbuild`. Previously, the upstream repository was the Vaticle's
fork of `rules_docker`. That fork contained a patch that fixed a bug in
`rules_pkg` that stopped `pkg_tar` rule from working with long paths
(the bug caused targz assembly of TypeDB to be incorrect) .
Incidentally, an identical patch was applied in Vaticle's
`bazel-distribution`. By loading, `rules_docker` from
`vaticle_dependencies` before `bazel-distribution`, we ended up using
the unpatched `rules_pkg` in the assembly targets that were depending on
`rules_pkg` (we can't have multiple versions of a workspace dependency
at the same time). This caused the targz assembly of TypeDB to contain
invalid paths.

The solution in this PR simply swaps the order of loading, so that
`typedb` ends up depending on the patched `rules_pkg` that is provided
by `bazel-distribution`.

The ideal solution for this problem would be to bump the `rules_pkg`
dependency in `bazel-distribution`, however, the newer versions of
`rules_pkg` changed their subpackage visibility modifiers. More
precisely, in the newer versions, `pkg_tar` depends on private
subpackages. Unfortunately, `stardoc`, the tool that we use to generate
documentation, does not work with rules that depend on rules that are
invisible to our project
(bazelbuild/stardoc#93,
bazelbuild/stardoc#128). Therefore, in order
to continue using `stardoc`, we must continue using the older version of
`rules_pkg`.

Co-authored-by: Lukas Slezevicius <[email protected]>
Co-authored-by: Alex Walker <[email protected]>
@tmhdgsn
Copy link

tmhdgsn commented Aug 29, 2023

has there been any progress on this at all? Currently unable to build because the rule to be documented uses java_binary/java_library from rules_java.

'documentation generation failed: File external/rules_java/java/defs.bzl imported '//java/private:native.bzl'

@tetromino
Copy link
Collaborator

tetromino commented Aug 29, 2023

I tried, but alas, it turned out to be too difficult given realistic time constraints. There are systems out there which rely on bazel query to produce the set of targets on which any given target - such as the .md file emitted by stardoc - depends. And bazel query can only query the target graph, not the loading graph; allowing it to query the loading graph without significantly degrading performance on large repos would be technically quite challenging (as in: quite probably several months of work). Which means we still have to express stardoc's deps in the target graph, not just in the loading graph. Which means we have to continue declaring in your BUILD file the deps of the .bzl file being documented.

I'm not closing this issue yet: although we must require access to dependencies in the Stardoc rule, it might be possible to have a separate command (which gets invoked from the terminal, not from a rule) for generating the doc. But that certainly will not happen before next year.

@tetromino
Copy link
Collaborator

has there been any progress on this at all? Currently unable to build because the rule to be documented uses java_binary/java_library from rules_java.

'documentation generation failed: File external/rules_java/java/defs.bzl imported '//java/private:native.bzl'

@tmhdgsn, please ensure that

  • your stardoc target has in "@rules_java//java:rules" in its transitive deps; and
  • you are using rules_java 5.3.2 or newer.

Does that fix the problem? If not, please add detailed repro steps and we will try to help debug.

@tmhdgsn
Copy link

tmhdgsn commented Aug 30, 2023

has there been any progress on this at all? Currently unable to build because the rule to be documented uses java_binary/java_library from rules_java.
'documentation generation failed: File external/rules_java/java/defs.bzl imported '//java/private:native.bzl'

@tmhdgsn, please ensure that

  • your stardoc target has in "@rules_java//java:rules" in its transitive deps; and
  • you are using rules_java 5.3.2 or newer.

Does that fix the problem? If not, please add detailed repro steps and we will try to help debug.

somehow I had managed to miss that target "@rules_java//java:rules", thank you !! all good now.

@hauserx
Copy link
Contributor

hauserx commented Jan 19, 2024

We also hit this problem as we include e.g. @bazel_tools//tools/cpp:toolchain_utils.bzl in some rules.
For now we used --spawn_strategy=local as a workaround, but it stopped working between stardoc 0.5.3 and 0.5.4 (where stardoc stopped to use --dep_roots flag and started to use runfiles, see ffcb4fb and bazelbuild/bazel@fde2479)

@tetromino you mentioned there can be some two staged approach possible, where we execute bazel first to gather all dependencies. How could it look like, how hard would it be to implement as a workaround?

Other possible solution would be to change starlark parser to allow undefined deps, possibly by traversing just starlark AST in stardoc, which could work in some cases and be available as a special mode of stardoc rule, but not sure about this solution for now.

@tetromino
Copy link
Collaborator

@hauserx - one approach would be for Bazel to gain a new non-build command or command-line option - perhaps a new option to bazel dump or some new flavor of bazel query - which will take a label of a .bzl file and dump out a proto that could be transformed into Markdown by Stardoc's renderer.

Open design questions:

  • what this command or option should be called
  • should it operate on a single .bzl file or multiple ones
  • should it output in Stardoc proto format, or do we also want to retrieve some additional loading-phase data for use by other tools
  • for rendering to markdown, do we recommend running the Stardoc renderer from the command line, or via the currently private stardoc_markdown_renderer rule (which would have to be cleaned up a bit and made public)?

@hauserx
Copy link
Contributor

hauserx commented Jan 26, 2024

Wonder whether it would be possible for bazel to create automatic targets for .bzl files with dependencies created out of load statements. Then stardoc rules could use regular aspects and harvest all dependencies, and need for bzl_library would be replaced by such mechanism.

The .bzl files by their nature are special, so possibly special handling of those could be possible. Already loadfiles function mentions something like that, maybe it's not only stardoc that would benefit from such approach: https://bazel.build/query/language#loadfiles

For posterity, for srcs from @bazel_tools you can create bzl_libray in your own workspace as below. Using it I was able to fix our doc generation to work in newer versions of stardoc.

bzl_library(
    name = "bazel_tools_bzl",
    srcs = ["@bazel_tools//tools:bzl_srcs"],
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants