mkdocs-material icon indicating copy to clipboard operation
mkdocs-material copied to clipboard

The `info` plugin does not includes inherited configurations

Open mondeja opened this issue 1 year ago β€’ 3 comments

Context

No response

Bug description

When you create a report with the info plugin having a INHERIT key in your Mkdocs configuration, the generated ZIP does not includes inherited configurations.

Related links

  • https://www.mkdocs.org/user-guide/configuration/#configuration-inheritance
  • https://squidfunk.github.io/mkdocs-material/plugins/info/

Reproduction

Create a minimal example, generate your report building and check that the inherited configuration file is not added.

Steps to reproduce

  1. mkdocs new .
  2. Change mkdocs.yml content by:
INHERIT: 'parent.yml'
theme: material
plugins:
    - info
  1. Create parent.yml file with the content:
site_name: Foo
  1. mkdocs build
  2. Give a name and extract the result.
  3. The parent.yml file is missing.

Browser

No response

Before submitting

mondeja avatar Feb 07 '24 23:02 mondeja

Oh, I forgot about that, I think I once said I will provide some hotfix πŸ˜“ So the current issues I know about are:

  • Not copying mkdocs.yml if it's not in the same directory as the executed mkdocs build command
  • Not following INHERIT paths Perhaps other quirks related to the project plugin and nested configs.

@squidfunk If you don't have any plans on tackling this issue over the next 1-2 weeks, I could attempt a fix and a PR. Is that acceptable?

kamilkrzyskow avatar Feb 11 '24 16:02 kamilkrzyskow

@kamilkrzyskow thanks for offering. I plan to fix it asap, but I'm currently too busy with #6307.

We need to think a little deeper about this, because we now also have the projects plugin that can contain multiple nested MkDocs projects, so the entire approach of collecting all files that are visible to MkDocs might not have been the best idea, because we can't catch all of them with that approach:

https://github.com/squidfunk/mkdocs-material/blob/25a663fd6a0484294f7340b37137aaf52d0380ec/src/plugins/info/plugin.py#L122-L125

I'm thinking of collecting all files and blacklisting those we don't want to pack, like site packages (if using a venv in the same folder), .DS_Store etc. Additionally, I think other folders containing Markdown files, e.g., when using Snippets, are currently not collected as well, so we need to find a way to do that.

I haven't started working on this, but if you like to, you're invited to do so!

squidfunk avatar Feb 12 '24 02:02 squidfunk

OK, then I shall see what I can do during the week and report back.

kamilkrzyskow avatar Feb 12 '24 07:02 kamilkrzyskow

@kamilkrzyskow did you manage to look into this?

squidfunk avatar Feb 23 '24 07:02 squidfunk

Oh, it's been 2 weeks already πŸ‘€, I missed both deadlines. Sorry about the delay @squidfunk. To update you about the current state of progress:

  • I added support for INHERIT handling in https://github.com/kamilkrzyskow/mkdocs-material/commit/7a5244b79f1aa133072b4a3292ee20e302421798
  • ...and later got stuck on a possible edge case when handling all files, and realised that handling INHERIT explicitly like I did might be redundant.

The idea was supposed to be process all files in the root instead of the files visible to MkDocs, right?

This would support the best case scenario, where all files are children of root:

Scenario 1 - All children
/some/path/mkdocs.yml
/some/path/inherit.yml
/some/path/requirements.txt
/some/path/snippets/abbreviations.md

/some/path/mkdocs build

and would also support the currently not supported scenario with nested config directory, as it's still a child of root:

Scenario 2 - All children but with nested configs
/some/path/configs/mkdocs.yml
/some/path/configs/inherit.yml
/some/path/requirements.txt
/some/path/snippets/abbreviations.md

/some/path/mkdocs build --config-file configs/mkdocs.yml

However, I'm not sure how to handle a case where the root is not a parent of the configs or other files. Given a more crazy scenario that perhaps isn't crazy with the projects directory, which I didn't look into yet, because the assumption was that handling all files from top to bottom would be enough:

Scenario 3 - Not all children
/some/path/configs/mkdocs-1.yml
/some/path/configs/mkdocs-2.yml
/some/path/configs/inherit.yml
/some/path/requirements.txt
/some/path/globals/snippets/abbreviations.md

/some/path/output/mkdocs build --config-file ../configs/mkdocs-1.yml --site-dir site-1

The user wants to use the config from a parent directory, while being inside the output directory, also the snippets are in globals, which isn't a child of /output/

So do I error message the user and ask them to run mkdocs build from the parent directory, do I quietly change the current working directory to the parent directory of the config, or do I add a plugin config option where the user has to specify the root, do I error message the user saying it's not a valid minimal project structure? None seem like a good idea, so therefore I got stuck.


You mentioned handling snippets, I thought that handling files top to bottom would be enough to include them, but looking at Scenario 3 the user might store the snippets externally, ~~so perhaps processing the YAML would be also required to get the path similarly to INHERIT.~~ EDIT: The markdown_extension configuration is in the MkDocs config, so parsing YAML isn't required in this instance.

As for excluding files, we could use https://github.com/github/gitignore/blob/main/Python.gitignore as reference and add some excluded cache files, I consider processing .gitignore files as outside of scope for minimal reproductions, but perhaps detecting if GitPython is installed and handling additional exclusions of a repository might be worthwhile for more advanced reproductions.

For users wanting more control, similarly to https://timvink.github.io/mkdocs-git-revision-date-localized-plugin/options/#exclude we could allow to add their own excluded paths.

So this is the current state of things, not looking great after 2 weeks πŸ˜“ I think the best solution would be to just enforce a structure where all reproduction files are required to be children of root, but let me know what you think :v:

kamilkrzyskow avatar Feb 23 '24 12:02 kamilkrzyskow

...and later got stuck on a possible edge case when handling all files, and realised that handling INHERIT explicitly like I did might be redundant.

I think we shouldn't rely on INHERIT, because this won't save us in case of other cases like the projects plugin, or multiple unrelated MkDocs configuration files.

The idea was supposed to be process all files in the root instead of the files visible to MkDocs, right?

Jup, because otherwise we loose files that are implicitly used, such as Snippets (used by the Snippets extension) or sources (used by mkdocstrings) + any cases I currently can't think of. Limiting inclusion to the files visible to MkDocs was quite naive of me, as we're missing out on too many cases, so we should depart from it. Additionally, packaging too much is not as severe as missing files for reproduction.

So do I error message the user and ask them to run mkdocs build from the parent directory, do I quietly change the current working directory to the parent directory of the config, or do I add a plugin config option where the user has to specify the root, do I error message the user saying it's not a valid minimal project structure? None seem like a good idea, so therefore I got stuck.

I haven't thought of the use cases where the MkDocs file is in a separate directory. I think the "config in subdirectory" case is rather rare, as the canonical recommended way is to put it at the top. Additionally, we demand minimal reproductions, which means the user can just change the structure for the reproduction (assuming the error doesn't lie within this very structure, but it is not recommended anyway).

As for excluding files, we could use https://github.com/github/gitignore/blob/main/Python.gitignore as reference and add some excluded cache files, I consider processing .gitignore files as outside of scope for minimal reproductions, but perhaps detecting if GitPython is installed and handling additional exclusions of a repository might be worthwhile for more advanced reproductions.

It should be quite safe to assume that we won't have a .gitignore available when the user creates a reproduction, but we can have our own that we use to exclude common stuff like .DS_Store and friends. Additionally, I think it will be mandatory to determine whether the site-packages folder is part of the root (when using a virtual environment), and exclude that as well, or the reproduction will include the entire Python runtime.

[...] the best solution would be to just enforce a structure where all reproduction files are required to be children of root

Yes, let's to that. I'd say we keep it as stupid and simple as possible:

  1. Collect all files recursively from current directory
  2. Filter files that are in our .gitignore or in site-packages
  3. Iterate and refine our .gitignore

The info plugin also shouldn't require us to add new dependencies, because everybody will install those dependencies on every download, which wastes time and resources – we need to keep the barrier as low as possible, and "just add - info to plugins" is probably the lowest we can go and where we should stay ☺️

squidfunk avatar Feb 24 '24 06:02 squidfunk

Keeping open until released.

squidfunk avatar Mar 05 '24 06:03 squidfunk

Released as part of 9.5.13. @kamilkrzyskow I had to issue a release today due to a pretty severe path computation bug in the projects plugin. We're moving the improvements you're cooking up in #6874 to the next release.

squidfunk avatar Mar 06 '24 06:03 squidfunk