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

The info plugin does not includes inherited configurations #6750

Closed
4 tasks done
mondeja opened this issue Feb 7, 2024 · 8 comments · Fixed by #6826
Closed
4 tasks done

The info plugin does not includes inherited configurations #6750

mondeja opened this issue Feb 7, 2024 · 8 comments · Fixed by #6826
Assignees
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open

Comments

@mondeja
Copy link
Contributor

mondeja commented Feb 7, 2024

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

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

@squidfunk squidfunk added the bug Issue reports a bug label Feb 8, 2024
@kamilkrzyskow
Copy link
Collaborator

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?

@squidfunk
Copy link
Owner

@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:

# Append all files visible to MkDocs
for file in get_files(config):
path = os.path.relpath(file.abs_src_path, os.path.curdir)
f.write(path, os.path.join(example, path))

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!

@kamilkrzyskow kamilkrzyskow self-assigned this Feb 12, 2024
@kamilkrzyskow
Copy link
Collaborator

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

@squidfunk
Copy link
Owner

@kamilkrzyskow did you manage to look into this?

@kamilkrzyskow
Copy link
Collaborator

kamilkrzyskow commented Feb 23, 2024

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 kamilkrzyskow@7a5244b
  • ...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 ✌️

@squidfunk
Copy link
Owner

squidfunk commented Feb 24, 2024

...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
Copy link
Owner

Keeping open until released.

@squidfunk squidfunk reopened this Mar 5, 2024
@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label Mar 5, 2024
@squidfunk
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants