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

add full and dev extras #1157

Merged
merged 2 commits into from
Jul 14, 2024
Merged

Conversation

branchvincent
Copy link
Contributor

This does two main things, split into separate commits. Main motivation for the PR is the first commit:

  • Adds a full extra mapping to requirements.txt so users can pip install volatility3[full]
    • We can't install leechcorepyc on macos since it's unsupported
    • We must strip the -r lines since they are not PEP 508 compliant
  • Adds a dev extra mapping to requirements-dev.txt so devs can pip install -e .[dev]

Second commit migrates all static metadata to pyproject.toml (the modern replacement for setup.py). Two things to note:

  • When setuptools reads the dynamic version, volatility3 won't be importable (due to build isolation) so I've moved the version constants to a separate file
  • I had to keep setup.py around just to strip to strip the -r lines for the new extras. If you don't mind changing the dev workflow from pip3 install -r requirements-dev.txt to pip3 install -e .[dev], we could remove setup.py entirely

@ikelos
Copy link
Member

ikelos commented Jun 4, 2024

Thanks very much, generally I'm happy with this (although I don't much like toml, it doesn't seem to interfere too much with anything else). People may already have their flows built around setup.py and I'm happy keeping it around. Volatility needs to support as far back as python 3.7, and I'm not sure when the pyproject/pip stuff got added.

In both instances I'll need to look into the build package to find out what it is and how it works, but generally looks pretty clean and modern. Gimme a few weeks to check it over (and then prod me if it hasn't gone anywhere) and that'll also give @npetroni chance to give it a look over (he helps me roll releases and helps look after pypi) to check that these changes won't impact anything too much, and also for @awalters to check he's happy with the metadata in the pyproject.toml file... 5:)

@ikelos
Copy link
Member

ikelos commented Jun 4, 2024

I also want to check why constants imports anything extra? The version shouldn't really require moving to another file, all the constants should be static and importable without the rest of the framework. I'd sooner fix that than split the version off into a separate file.

@ikelos
Copy link
Member

ikelos commented Jun 4, 2024

The -r lines are to reduce redundancy by referencing the previous subset of required packages. My understanding was that it was part of the requirements.txt format. If there's an easier way of not duplicating requirements (so that you could accidentally update one but not the rest) then I'm happy to go with that. Since we're keeping the setup.py though, it's not an urgent issue.

@branchvincent
Copy link
Contributor Author

Thanks for taking a look!

Volatility needs to support as far back as python 3.7, and I'm not sure when the pyproject/pip stuff got added.

Support was added in pip 19, released Jan 2019 pypa/pip@3a77bd6 and included in python 3.7.3

I'll need to look into the build package to find out what it is and how it works

Basically, it reads the build-backend from pyproject.toml and calls methods on that package to create an sdist and wheel. PEP 517 is the official specification of this build process and pip has a nice summary

I also want to check why constants imports anything extra?

It's from forwarding these imports:

import volatility3.framework.constants.linux
import volatility3.framework.constants.windows

which are used in places like

if name == constants.linux.KERNEL_NAME:

I can remove those imports if you want instead, just didn't originally since it would be a breaking change

My understanding was that it was part of the requirements.txt format. If there's an easier way of not duplicating requirements (so that you could accidentally update one but not the rest) then I'm happy to go with that

pip understands them, but nothing else since it's not standardized. The standard way is defining requirements directly in your pyproject.toml file like:

[project]
dependencies = ["pefile>=2023.2.7"]

[project.optional-dependencies]
dev = ["jsonschema>=2.3.0", "..."]
full = ["yara-python>=3.8.0", "..."]
all = ["volatility3[dev,full]"] # we can reference other extras like so

and then you can install any combination you want, like pip install .[full] or pip install .[full,dev]

@branchvincent
Copy link
Contributor Author

hey @ikelos, just checking in 😄

@ikelos
Copy link
Member

ikelos commented Jul 4, 2024

I think we're very happy with your changes and we'll likely get them merged, we're just talking about either bumping the minimum python version required or figuring out how to still keep it all working without pip. Definitely not forgotten though, sorry, I'm just a bit slower at getting through things these days... 5:S

@branchvincent
Copy link
Contributor Author

No worries! Since we're keeping setup.py, note that users won't be forced to use pip as long as they have a recent enough setuptools (I tested back to setuptools==62.6.0):

$ python3.8 -m venv .venv
$ .venv/bin/python -m pip install setuptools==62.6.0
$ .venv/bin/python setup.py install
$ head -1 volatility3.egg-info/requires.txt 
pefile>=2023.2.7 # <-- this came from the pyproject.toml file

But bumping the python version sounds good to me as well

@ikelos ikelos merged commit d334525 into volatilityfoundation:develop Jul 14, 2024
14 checks passed
@branchvincent branchvincent deleted the full-extra branch July 14, 2024 23:18
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.

4 participants