Skip to content

Tox Tests Failing Due to Integration of Volatility Submodule #54

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

Closed
Ston14 opened this issue Jul 25, 2024 · 3 comments
Closed

Tox Tests Failing Due to Integration of Volatility Submodule #54

Ston14 opened this issue Jul 25, 2024 · 3 comments
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@Ston14
Copy link
Contributor

Ston14 commented Jul 25, 2024

Description:

Since the latest changes, the tests using Tox are no longer functioning. The issue appears to be caused by the integration of the Volatility submodule.

Steps to Reproduce:

  1. Pull the latest changes from the repository.
  2. Run the tests using Tox.

Current Behavior:
The tests fail after integrating the Volatility submodule.

Expected Behavior:
The tests should run successfully without any errors.

@Ston14 Ston14 added bug Something isn't working help wanted Extra attention is needed labels Jul 25, 2024
@Ston14 Ston14 self-assigned this Jul 25, 2024
@Ston14 Ston14 added this to Features Jul 25, 2024
@Ston14 Ston14 moved this from New to Planned queue in Features Jul 25, 2024
@YannMagnin
Copy link
Contributor

poetry install will follow the submodule path but as explained in my PR #50, the pyproject.toml which Volatility3 exposes and which poetry will target rely on a mechanism from setuptools called dynamic that allow modular installation, and by default the package will only install the required dependencies but not all of them. In our case, we need to install all of the dependencies of the project to allow the use of some plugins, but poetry does not support dynamic configuration. So, we cannot simply indicate to poetry the module path.

The use of the build_submodules.py is a good idea but, for me, it should be more transparent and more straightforward to merge its content to the pydfirram/__init__.py to automatically check if the submodule has been installed or not.

I can change my PR to implement this if you want, it will be much more cleaner, but I need to know your thoughts:

  • do we need to indicate (using print() ?) to the user each step of the submodule installation when it occurs ?
  • can we remove the submodule since we can manually clone the repository during the step ?
  • are we sure that this is the cleanest way to reach our goal, because it seems a little hacky ?

@Ston14
Copy link
Contributor Author

Ston14 commented Aug 8, 2024

Hi @YannMagnin,

Thanks for your PR #50 and for the thoughtful suggestions.

After considering the options, I believe it's best to maintain simplicity and stability in our project by sticking with the standard Volatility package from PyPi. Here are some key points:

Pros of Using the Standard Package:

  • Simplicity: The installation process remains straightforward and user-friendly, avoiding the complexity of managing a submodule.
  • Stability: By using a stable version from PyPi, we reduce the risk of introducing bugs or compatibility issues that might arise from using a constantly updated submodule.
  • Ease of Maintenance: This approach simplifies maintenance as we don’t need to track and update a submodule or manage additional installation scripts.

Cons:

  • Delayed Updates: We may not have immediate access to the latest features or plugins until the Volatility developers update their package on PyPi.
  • Additional Documentation: Users will need clear instructions on how to add and use new plugins, which requires good documentation.

Given these points, I’m considering reverting to the previous setup where we used the Volatility3 package directly in the pyproject.toml and poetry.lock, rather than keeping the current submodule solution. This would align with our goal of simplicity and stability.

I’d like to get your opinion on this approach. Do you think it would be a better solution to go back to using the package directly, or do you have other thoughts on how we should proceed?

Thanks again for your contribution!

@Ston14
Copy link
Contributor Author

Ston14 commented Aug 19, 2024

Hi @YannMagnin,

I wanted to let you know that the issue has been resolved in my latest PR.

I’ve implemented the changes we discussed:

  • Removed the Volatility submodule and the associated script for fetching it.
  • Reverted to using the standard Volatility3 package from PyPi, as suggested, to maintain simplicity and stability.
  • Updated the tests to ensure they are now fully functional with Tox.

This should streamline the installation and maintenance process while reducing potential issues with updates.

I will be closing your PR as well as this issue, given that the necessary changes have been made. Thanks again for your initial suggestions and contributions!

Best regards

@Ston14 Ston14 closed this as completed Aug 19, 2024
@github-project-automation github-project-automation bot moved this from Planned queue to Done in Features Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
Archived in project
Development

No branches or pull requests

2 participants