volatility3 icon indicating copy to clipboard operation
volatility3 copied to clipboard

add full and dev extras

Open branchv opened this issue 1 year ago • 5 comments

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

branchv avatar Jun 01 '24 23:06 branchv

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 avatar Jun 04 '24 22:06 ikelos

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 avatar Jun 04 '24 22:06 ikelos

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.

ikelos avatar Jun 04 '24 22:06 ikelos

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 https://github.com/pypa/pip/commit/3a77bd667cc68935040563e1351604c461ce5333 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:

https://github.com/volatilityfoundation/volatility3/blob/771ed10b44573a7f8baa32822f3bc524195fe0c9/volatility3/framework/constants/init.py#L15-L16

which are used in places like

https://github.com/volatilityfoundation/volatility3/blob/771ed10b44573a7f8baa32822f3bc524195fe0c9/volatility3/framework/symbols/linux/init.py#L335

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]

branchv avatar Jun 05 '24 03:06 branchv

hey @ikelos, just checking in 😄

branchv avatar Jun 30 '24 17:06 branchv

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

ikelos avatar Jul 04 '24 22:07 ikelos

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

branchv avatar Jul 08 '24 00:07 branchv