Added file-collection options for external/generated/data in deps

[mikael-s-persson]

Added file-collection options for external/generated/data in deps

Generally, it's not desirable to follow dependencies into external repositories, generated files or data dependencies because those are unlikely to be what the user wants to document and are likely to include a very large amount of stuff.

This PR just makes the default aspect to collect files stop at external dependencies, filter out generated source files, and not follow data dependencies (which are typically binary or configuration files needed at run-time, not source files). Those can be turned on again by passing a custom doxygen rule (from a custom aspect) to the "doxygen" macro.

Of course, as an alternative, a user could pass exclusion patterns to doxygen via its own options for that, which means the aspect still collects everything, but doxygen ignores stuff in the end. That's not terrible since the aspect just collects files, which is pretty cheap. But there might some very good reasons in the future to do more work in the aspect (such as incrementally generating doxygen docs on a per-target basis, which is the main thing I have in mind, because running doxygen once in the end on everything quickly becomes unsustainable on a mid-size project or bigger).

[TendTo]

I find the general idea quite interesting, and I do think that allowing the user to personalize the aspect or the rule is a valuable addition. I am just a bit torn on the specific application. I am unsure whether I want to add this kind of logic into rules_doxygen, as it is something that, as you said, Doxygen can, and in my view should, handle. The philosophy of this rule is to be a bare-bone middleman between bazel rules and Doxygen, to make it more convenient to use.

I suppose the pre-filter due to the provider could have a meaningful impact on very large projects, but in that case I would rather keep the new pluggable system you have created and let the user themselves design the aspect/rule they need, tailored to their use cases, leaving your specific implementation as a neat example.
Let me know what you think.

There is also the failing github actions, but it seems that is something related to bazel 7.0.0 support. It is probably something relatively easy to address.

The other change request are very nitpicks I would have generally done myself by pushing on your branch, but since I don't have the permissions, I can apply the changes after the pr merge.
I have pushed some minor changes to the main branch, but you should be able to rebase on that with no issues.

[mikael-s-persson]

I suppose the pre-filter due to the provider could have a meaningful impact on very large projects, but in that case I would rather keep the new pluggable system you have created and let the user themselves design the aspect/rule they need, tailored to their use cases, leaving your specific implementation as a neat example.
Let me know what you think.

For context, I plan to provide a doxygen aspect runner for my bazel_cc_meta repo. Currently, that repo runs an aspect on cc_* targets that generates a compile_commands.json file (for clangd/clang-tidy) and other info such as imports and exports to automatically fix dependency issues. I plan to run doxygen (and some other related tools) through an aspect as well, similar to my recent addition of a clang-tidy aspect. I believe it should be possible to use the same workflow with doxygen through its tag-files system. And I also want to benefit from clang-assisted-parsing and employing per-target compile-commands files (the way I do in the clang-tidy aspect).

This particular PR is really more of a fly-by fix when I noticed that your file-collection aspect wasn't filtering out external, generated or data files, which I really think it should not include by default, mostly because I think wanting to include those things is the exception rather than the rule for users, and there are many ugly pitfalls related to them (from my experience writing bazel_cc_meta and many other aspects and rules). Take it or leave it, I don't really care that much, because I don't plan to use this repo's macros/rules for running doxygen, only its mechanisms for pulling in a hermetic distribution of doxygen.

More broadly speaking, the core philosophy of Bazel is to have localized, hermetic, cached build steps within a bigger build graph. Having a single top-level target that runs a global action on all files is really undesirable because you lose caching almost completely and also cause a bottleneck in distributed builds (sending all artifacts out and back again). So, I would encourage you to think about breaking this up or providing more decomposable mechanisms for users who might want to incrementally generated the docs. I might have other PRs to submit along those lines too. And I generally think that having the underlying scaffolding of aspect-factory + rule-factory + macro-with-sensible-defaults is going to be more flexible for you in the future so you can separate target-traversal options (passed to aspect factory), build options (passed to rule factory) and doxygen-config options (passed to macro), and potentially letting users customize them.

BTW, if you want users to be able to write their own aspect (not just provide a configurable aspect via a factory), then you'd have to make the provider less generic so it doesn't conflict with other providers that users might have to load as well.

I am unsure whether I want to add this kind of logic into rules_doxygen, as it is something that, as you said, Doxygen can, and in my view should, handle.

Here is a basic example. Say I have a cc_binary target which has a run-time data dependency which is a 5GB zip file of game assets, if the aspect picks up that file (and I can't configure it not to), and I add an exclusion to my doxyfile config to ignore that zip file, then Bazel will still consider that file as an input to the doxygen build rule. That means, it has to compute and compare its hash for caching, it will rebuild the doxygen if the assets change, it has to potentially send that file over to the remote build executor, and so on, all for nothing because it'll be ignore anyway. It's always preferable to filter things out as early as possible.

There is also the failing github actions, but it seems that is something related to bazel 7.0.0 support. It is probably something relatively easy to address.

Fixed that, I guess v7 is still defaulting to WORKSPACE, so I had to add that to the greeting "external" repository. BTW, version 7.0.0 doesn't actually work (rules_python 1.9.0 fails), it only works from 7.1.0 and above, at least, that's what it looks like when I try to pin the bazel version locally. I think that CI runners like those on the BCR run the latest of each major version (so "7.*" right now is going to test "7.7.1").

[TendTo]

This particular PR is really more of a fly-by fix when I noticed that your file-collection aspect wasn't filtering out external, generated or data files, which I really think it should not include by default, mostly because I think wanting to include those things is the exception rather than the rule for users, and there are many ugly pitfalls related to them (from my experience writing bazel_cc_meta and many other aspects and rules).

More broadly speaking, the core philosophy of Bazel is to have localized, hermetic, cached build steps within a bigger build graph. Having a single top-level target that runs a global action on all files is really undesirable because you lose caching almost completely and also cause a bottleneck in distributed builds (sending all artifacts out and back again). So, I would encourage you to think about breaking this up or providing more decomposable mechanisms for users who might want to incrementally generated the docs. I might have other PRs to submit along those lines too. And I generally think that having the underlying scaffolding of aspect-factory + rule-factory + macro-with-sensible-defaults is going to be more flexible for you in the future so you can separate target-traversal options (passed to aspect factory), build options (passed to rule factory) and doxygen-config options (passed to macro), and potentially letting users customize them.

Here is a basic example. Say I have a cc_binary target which has a run-time data dependency which is a 5GB zip file of game assets, if the aspect picks up that file (and I can't configure it not to), and I add an exclusion to my doxyfile config to ignore that zip file, then Bazel will still consider that file as an input to the doxygen build rule. That means, it has to compute and compare its hash for caching, it will rebuild the doxygen if the assets change, it has to potentially send that file over to the remote build executor, and so on, all for nothing because it'll be ignore anyway. It's always preferable to filter things out as early as possible.

That is a very good point, clearly coming from experience. I'm still a little tempted to just delegate this to the user giving them full control over the collection aspect. While this customization option is something I really want to include regardless, maybe the user experience would improve if I do provide a more sensible default. I will think about it a little bit more.

BTW, if you want users to be able to write their own aspect (not just provide a configurable aspect via a factory), then you'd have to make the provider less generic so it doesn't conflict with other providers that users might have to load as well.

I am curious about this point. By less generic, do you mean renaming TransitiveSourcesInfo into something like DoxygenRulesTransitiveSourcesInfo, to avoid conflict with other providers? Is there no namespacing for providers in Bazel? Or is it something else?

Fixed that, I guess v7 is still defaulting to WORKSPACE, so I had to add that to the greeting "external" repository. BTW, version 7.0.0 doesn't actually work (rules_python 1.9.0 fails), it only works from 7.1.0 and above, at least, that's what it looks like when I try to pin the bazel version locally. I think that CI runners like those on the BCR run the latest of each major version (so "7.*" right now is going to test "7.7.1").

Yeah, some check cannot run in 7.0.0, as you have noticed, that's why I skip them in the github action. But for those that can, I have pinned the 7.0.0 version, to ensure that rules_doxygen supports it.

[mikael-s-persson]

I am curious about this point. By less generic, do you mean renaming TransitiveSourcesInfo into something like DoxygenRulesTransitiveSourcesInfo, to avoid conflict with other providers? Is there no namespacing for providers in Bazel? Or is it something else?

Yeah, that's exactly what I meant. AFAIK, there are no namespaces in Bazel. Technically, you can get around it by renaming things when loading them, like load("//my_package:defs.bzl", BarInfo = "FooInfo") to load the FooInfo provider but refer to it as BarInfo within the bzl/build file where you load it. However, I'm not sure that's fool-proof (especially considering that they might clash as providers within a target, e.g., when doing target[TransitiveSourcesInfo], I'm not sure how that works if some other unrelated aspect also ran that also provides a provider with the same name). I'm happy to rename it in this PR. I think DoxygenCollectedSourcesInfo is a good name. I pushed that change.

While this customization option is something I really want to include regardless, maybe the user experience would improve if I do provide a more sensible default.

One more thing to consider, which is what I do in bazel_cc_meta is to have a tag that users can use to mark certain targets to be skipped. In the aspect, you could check for a tag called doxygen_skip, so that if users add tags = ["doxygen_skip"], to a target, the default collector aspect will skip that target (and its dependencies, or maybe not). That's generally a good scalable way to allow users to exclude some targets without having to list them out in the top-level "doxygen" target.

[TendTo]

One more thing to consider, which is what I do in bazel_cc_meta is to have a tag that users can use to mark certain targets to be skipped. In the aspect, you could check for a tag called doxygen_skip, so that if users add tags = ["doxygen_skip"], to a target, the default collector aspect will skip that target (and its dependencies, or maybe not). That's generally a good scalable way to allow users to exclude some targets without having to list them out in the top-level "doxygen" target.

Yep, that sounds really good. Once you are satisfied with the pr, I guess I will just merge it, maybe quickly go over the documentation (this is still a potentially breaking change, although I doubt it will affect anyone) and then publish the new rules version 2.7.0

[mikael-s-persson]

Yep, that sounds really good. Once you are satisfied with the pr, I guess I will just merge it, maybe quickly go over the documentation (this is still a potentially breaking change, although I doubt it will affect anyone) and then publish the new rules version 2.7.0

Sorry for the delay, business trip. I added the doxygen_skip tag to exclude specific targets (and added that to examples). And I updated the docs to explain the new default behavior and new aspect/rule parameters. I think this PR is pretty much good to go.