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

Remove indirect dependencies #4873

Open
thinrope opened this issue Apr 10, 2024 · 11 comments · May be fixed by #4874
Open

Remove indirect dependencies #4873

thinrope opened this issue Apr 10, 2024 · 11 comments · May be fixed by #4874
Assignees
Labels
maintenance Issue related to project maintenance

Comments

@thinrope
Copy link
Contributor

thinrope commented Apr 10, 2024

While working on packaging latest plaso for Gentoo, I think I see two python modules (idna,cffi) which are not referenced in any code, yet they are installed by some of the scripts. I suggest to have them removed from dependencies.ini and requirements.txt.

Those are only indirect dependencies as follows:

  • idna <- requests
  • cffi <- dfvfs TBC!

This concerns plaso-20240409 on Gentoo installed from my pkalin overlay, as there is no official plaso support on Gentoo yet.

NOTE: Description was edited as follows:

  • remove mention of cryptography (patch had to be reverted anyway)
  • explain those are indirect dependencies
@joachimmetz
Copy link
Member

cryptography should be no longer there, idna and cffi we would need to check.

@joachimmetz joachimmetz self-assigned this Apr 10, 2024
@joachimmetz joachimmetz added the maintenance Issue related to project maintenance label Apr 10, 2024
thinrope added a commit to thinrope/plaso that referenced this issue Apr 10, 2024
log2timeline/plaso/log2timeline#4873 WIP

Bug: log2timeline#4873
Reported-by: Kalin KOZHUHAROV <kalin@thinrope.net>
Signed-off-by: Kalin KOZHUHAROV <kalin@thinrope.net>
thinrope added a commit to thinrope/plaso that referenced this issue Apr 10, 2024
log2timeline/plaso/log2timeline#4873 WIP

Bug: log2timeline#4873
Reported-by: Kalin KOZHUHAROV <kalin@thinrope.net>
Signed-off-by: Kalin KOZHUHAROV <kalin@thinrope.net>
thinrope added a commit to thinrope/plaso that referenced this issue Apr 10, 2024
log2timeline/plaso/log2timeline#4873 WIP

Bug: log2timeline#4873
Reported-by: Kalin KOZHUHAROV <kalin@thinrope.net>
Signed-off-by: Kalin KOZHUHAROV <kalin@thinrope.net>
@thinrope thinrope linked a pull request Apr 10, 2024 that will close this issue
3 tasks
thinrope added a commit to thinrope/plaso that referenced this issue Apr 10, 2024
It looks like it is relying on a hard-coded output that just happens to
have those module names?
log2timeline/plaso/log2timeline#4873 WIP

Bug: log2timeline#4873
Reported-by: Kalin KOZHUHAROV <kalin@thinrope.net>
Signed-off-by: Kalin KOZHUHAROV <kalin@thinrope.net>
@joachimmetz
Copy link
Member

joachimmetz commented Apr 11, 2024

looks like idna was a requirement of cryptography and requests, cryptography is no longer used, but let me check requests

@joachimmetz
Copy link
Member

joachimmetz commented Apr 11, 2024

Looks like idna still is a dependency of requests https://github.com/psf/requests/blob/main/setup.cfg#L6 . I would need to look why it was explicitly added instead of it being an implicit dependency. I recommend we keep that one for now.

@joachimmetz
Copy link
Member

And cffi is a dependency of xattr https://github.com/xattr/xattr/blob/main/pyproject.toml#L15

@joachimmetz
Copy link
Member

I would need to assess first, the reason for these indirect dependencies were added. Likely due to one of the packaging/environments failing to include them.

@joachimmetz
Copy link
Member

Looks like cffi was added for cryptography but this predates the inclusion of xattr 221a298

@joachimmetz
Copy link
Member

Idna was explicitly added for requests 8871cd9

@thinrope
Copy link
Contributor Author

I'll work on this a bit more, but here are my thoughts:

  • no indirect deps should be present in dependencies.ini and requirements.txt, they may be in some of the CICD files
  • given that I removed those 3 and tests passed, we need better tests :-) May be something that uses IDNA url for example

@joachimmetz
Copy link
Member

no indirect deps should be present

Unfortunately the "should" here is one of those theory versus reality arguments. They were added because they were needed.

  • Given they are used indirectly anyway, they not abundant, we can take the time to take a closer look why they were originally added
  • Many things have changed since they were added it could be that they are no longer needed.

we need better tests :-)

tests can always be improved

@joachimmetz joachimmetz changed the title Cleaning some unused dependencies Remove indirect dependencies Apr 14, 2024
@thinrope
Copy link
Contributor Author

I am digging around and so far I don't see any direct use of cffi nor idna across the projects even:

(Disclaimer: The above two ways are the only standard ways to use a Python module, AFAIK)

While dfvfs also lists cffi as direct dep, I see no place where it is used. Based on blame, cffi is a remains of pycrypto->cryptography move 5 years ago (and now cryptography is no longer used). Let me know if we need another issue for this (since different project).

@joachimmetz
Copy link
Member

(Disclaimer: The above two ways are the only standard ways to use a Python module, AFAIK)

Python has many "standard" ways of using modules.

I'll have a closer looks when time permits. Maybe this was msi or Pyinstaller related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Issue related to project maintenance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants