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

Fix dependency conflicts in pyproject.toml #1548

Closed
wants to merge 2 commits into from

Conversation

dgmcdona
Copy link
Contributor

@dgmcdona dgmcdona commented Jan 14, 2025

Closes #1546

This makes three changes to fix dependency resolution

  • Fixes a bug where the sphinx version is unresolvable, and pins it to
    8.0.2 (the minimum version required by the minimum version of
    sphinx-autodoc-typehints)
  • Lowers the required version of pefile in order to allow us to use
    the pyinstaller version specified in the dev dependencies
  • Bumps the python version to 3.10 in order to support sphinx 8.0.2

I know bumping the minimum Python version is not desirable, but I wasn't sure which sphinx/sphinx-autodoc-typehints versions would be acceptable due to my unfamiliarity with our doc requirements. We can lower those if an acceptable version can be found that supports python 3.8. There isn't a way to specify different required Python versions for different sets of dependencies within pyproject.toml.

This makes three changes to fix dependency resolution
- Fixes a bug where the sphinx version is unresolvable, and pins it to
  8.0.2 (the minimum version required by the minimum version of
  sphinx-autodoc-typehints)
- Lowers the required version of `pefile` in order to allow us to use
  the `pyinstaller` version specified in the `dev` dependencies
- Bumps the python version to 3.10 in order to support sphinx 8.0.2
@dgmcdona
Copy link
Contributor Author

I reverted the Python version bumps and found the highest versions of sphinx-autodoc-typehints and sphinx that still support Python 3.8. Installing those versions and building the docs with cd doc; make html seemed to work fine, although there are some existing syntax errors within the documentation that don't cause the build to fail.

I also looked through the git history to see if there was a reason that we require such a high version of pefile and couldn't find one, it seems that it was bumped in this commit

This lowers the required versions of sphinx and sphinx-autodoc-typehints
to the highest versions that still support Python 3.8.
Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think this messes with a few too many things. Many of the requirements were artificially bumped up by the person pushing up to move to pyproject.toml, but I'm not sure there's good cause for blocking the older versions.

I also don't really want to fix on an old version of pefile because pyinstaller (used once and only by devs) has a performance issue for the one time we run it.

@@ -11,7 +11,7 @@ license = { text = "VSL" }
dynamic = ["version"]

dependencies = [
"pefile>=2024.8.26",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong way to fix the issue. I don't think pefile is at fault, I believe it's the way that pyinstaller used it, and pyinstaller is only used by devs to construct an exe (and it's only a performance degradation), so I think we bump pyinstaller down (it used to start at 6.5, but during the toml update it got bump to the latest version at that time).

@@ -48,8 +48,8 @@ test = [

docs = [
"volatility3[dev]",
"sphinx>=8.0.0,<7",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested and it appears to builds with version 8 ok, so I'm not sure this needs to drop to 7, I think the higher version should bump to 9.

@@ -48,8 +48,8 @@ test = [

docs = [
"volatility3[dev]",
"sphinx>=8.0.0,<7",
"sphinx-autodoc-typehints>=2.5.0,<3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this change is to work with the version of sphinx available? Again, I believe the lower bound was pulled up artificially when the pyproject.toml inhereited the values from the requirements.txt.

@ikelos
Copy link
Member

ikelos commented Jan 17, 2025

Could you please review #1558, and if you've got the ability to easily check the dependencies and outputs, check that everything's ok? I've left my comments for the reasons I took a different route in this PR, but feel free to point out problems in the new one, or reply here if you're not happy with any of the reasoning... Thanks!

@dgmcdona dgmcdona closed this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyproject.toml: dependency conflicts
2 participants