Skip to content

Prefer importlib.metadata over pkg_resources if available#2081

Merged
sentrivana merged 12 commits intomasterfrom
ivana/remove-pkg-resources
May 22, 2023
Merged

Prefer importlib.metadata over pkg_resources if available#2081
sentrivana merged 12 commits intomasterfrom
ivana/remove-pkg-resources

Conversation

@sentrivana
Copy link
Contributor

@sentrivana sentrivana commented May 5, 2023

Let's not use pkg_resources and avoid a DeprecationWarning if importlib.metadata is available.

Fixes #2048

sentrivana
sentrivana commented May 5, 2023
View reviewed changes
sentry_sdk/integrations/modules.py Outdated
from importlib.metadata import distributions, version

for dist in distributions():
yield dist.metadata["Name"], version(dist.metadata["Name"])
Copy link
Contributor Author

@sentrivana sentrivana May 5, 2023

Choose a reason for hiding this comment

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

There's also dist.name (it's just an alias of dist.metadata["Name"]), but it hasn't always been there. dist.metadata["Name"] should work with all py versions.

@sentrivana sentrivana force-pushed the ivana/remove-pkg-resources branch from 74f8a87 to 15893cf Compare May 5, 2023 12:58
@sentrivana sentrivana marked this pull request as ready for review May 5, 2023 12:58
@sentrivana sentrivana force-pushed the ivana/remove-pkg-resources branch from 15893cf to 4347c32 Compare May 5, 2023 13:09
antonpirker
antonpirker approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

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

If distributions() yields the same stuff as pkg_resources.working_set this looks good and can be merged.

@sentrivana
Copy link
Contributor Author

sentrivana commented May 8, 2023

If distributions() yields the same stuff as pkg_resources.working_set this looks good and can be merged.

@antonpirker It does with one difference: distributions() keeps the capitalization, working_set makes everything lowercase, e.g.:

# distributions()
MarkupSafe 2.1.2
matplotlib-inline 0.1.6
mock 5.0.2
packaging 23.1
parso 0.8.3
pexpect 4.8.0
pickleshare 0.7.5
pip 23.1.2
platformdirs 3.5.0
pluggy 0.13.1
prompt-toolkit 3.0.38
ptyprocess 0.7.0
pure-eval 0.2.2
py 1.11.0
Pygments 2.15.1

# working_set
markupsafe 2.1.2
matplotlib-inline 0.1.6
mock 5.0.2
packaging 23.1
parso 0.8.3
pexpect 4.8.0
pickleshare 0.7.5
pip 23.1.2
platformdirs 3.5.0
pluggy 0.13.1
prompt-toolkit 3.0.38
ptyprocess 0.7.0
pure-eval 0.2.2
py 1.11.0
pygments 2.15.1

Do you see this creating issues anywhere? I grepped for where we actually use the modules and it seems like we attach them to the event and check them here but elsewhere we seem to be using sys.modules instead.

@antonpirker
Copy link
Contributor

antonpirker commented May 9, 2023

@sentrivana could you please add a test for testing _generate_installed_modules?

@sentrivana sentrivana force-pushed the ivana/remove-pkg-resources branch from ccb7d14 to 86b6f7d Compare May 11, 2023 13:13
antoni-szych-rtbhouse
antoni-szych-rtbhouse reviewed May 16, 2023
View reviewed changes
@sentrivana
Copy link
Contributor Author

sentrivana commented May 22, 2023

@antonpirker Changed the PR a bit and added a test, could you please skim over to see if it still looks ok to you?

antonpirker
antonpirker reviewed May 22, 2023
View reviewed changes
sentry_sdk/integrations/modules.py Outdated

def _normalize_module_name(name):
# type: (str) -> str
return name.lower().replace("-", "_")
Copy link
Contributor

@antonpirker antonpirker May 22, 2023

Choose a reason for hiding this comment

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

Why do we need to replace the "-" with a "_"?
Wouldn't we then report a package name "sentry_sdk" that is different than the one the user installs "sentry-sdk"?

Copy link
Contributor Author

@sentrivana sentrivana May 22, 2023

Choose a reason for hiding this comment

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

There's a weird mismatch between importlib and pkg_resources where they report the package typing-extensions differently; importlib reports typing_extensions, while pkg_resources reports it as typing-extensions. This is the only package that is different (if we normalize to lowercase, which we're doing). I'll remove the .replace() and change the test to ignore the one package.

Copy link
Contributor

@antonpirker antonpirker May 22, 2023

Choose a reason for hiding this comment

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

I guess if we just try to keep as much the same as it was reported with pkg_resources we are good to go.

@sentrivana sentrivana merged commit 8c24d33 into master May 22, 2023
@sentrivana sentrivana deleted the ivana/remove-pkg-resources branch May 22, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@antoni-szych-rtbhouse antoni-szych-rtbhouse antoni-szych-rtbhouse left review comments

@antonpirker antonpirker antonpirker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pkg_resources is deprecated as an API

3 participants

@sentrivana @antonpirker @antoni-szych-rtbhouse