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

Update package metadata opentelemetry-api #2867

Merged
merged 12 commits into from
Aug 25, 2022

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Aug 9, 2022

Background

Hello there! The Python packaging ecosystem has standardized on the interface for build backends (PEP 517/PEP 660) and the format for metadata declaration (PEP 621/PEP 631). As a result, the execution of setup.py files is now deprecated.

So, I'm spending my free time updating important projects so that they are modernized and set an example for others 😄

Description

This implements PEP 621 for opentelemetry-api, obviating the need for setup.py, setup.cfg, and MANIFEST.in. Our tooling already supports this. I'll open one PR per package after this one.

The build backend hatchling (of which I am a maintainer in the PyPA) is now used as that is the default in the official Python packaging tutorial. Hatchling is available on all the major distribution channels such as Debian, Fedora, Arch Linux, conda-forge, Nixpkgs, Alpine Linux, FreeBSD, Gentoo Linux, MacPorts, OpenEmbedded, Spack, etc.

Type of change

  • This change requires a documentation update

How Has This Been Tested?

tox -e py38-opentelemetry-api

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ofek ofek requested a review from a team August 9, 2022 05:45
@ofek
Copy link
Contributor Author

ofek commented Aug 9, 2022

@srikanthccv Hello! Should we add the Skip Changelog label?

@srikanthccv
Copy link
Member

@ofek probably not since this doesn't change anything for an end user but we might want to discuss the dropping support for 3.6. We decided to keep it until around metrics SDK becomes stable enough.

@ofek
Copy link
Contributor Author

ofek commented Aug 10, 2022

No worries, in that case I can keep the existing cap and only require 3.7+ in CI/CD. How does that sound?

@srikanthccv
Copy link
Member

@lzchen @ocelotl what do you think?

@lzchen
Copy link
Contributor

lzchen commented Aug 12, 2022

No worries, in that case I can keep the existing cap and only require 3.7+ in CI/CD. How does that sound?

sgtm

@ofek
Copy link
Contributor Author

ofek commented Aug 13, 2022

Pushed! Would you mind triggering the CI with that button below?

@ofek
Copy link
Contributor Author

ofek commented Aug 15, 2022

Only the changelog job is failing.

@lzchen lzchen added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 15, 2022
@ofek
Copy link
Contributor Author

ofek commented Aug 15, 2022

Failing job is just a flake for a performance regression

@lzchen lzchen closed this Aug 16, 2022
@lzchen lzchen reopened this Aug 16, 2022
@ofek
Copy link
Contributor Author

ofek commented Aug 16, 2022

Everything is passing now! Though it looks like the 3.6 jobs are hard coded in this repo's GitHub settings as required/expected.

@ofek
Copy link
Contributor Author

ofek commented Aug 17, 2022

There was a conflict due to a typo fix that just got merged so I rebased. As far as I'm reading, merging might actually remove the required 3.6 checks rather than having to edit the repo settings 🙂

@srikanthccv
Copy link
Member

No, the merge can only be done after "required" checks are successful. So this requires updating settings.

@ofek
Copy link
Contributor Author

ofek commented Aug 19, 2022

Oh okay, just let me know how I can help!

@lzchen
Copy link
Contributor

lzchen commented Aug 22, 2022

@ofek
Question for the future:

If we wanted to update opentelemetry-instrumentation to use pyproject.toml, according to PEP621:

Build back-ends MUST raise an error if the metadata defines a [project.entry-points.console_scripts] or [project.entry-points.gui_scripts] table, as they would be ambiguous in the face of [project.scripts] and [project.gui-scripts], respectively.

What would be your recommendation in how to define an entry point with the console_scripts table? Just project.scripts?

@ofek
Copy link
Contributor Author

ofek commented Aug 22, 2022

[project.scripts]
opentelemetry-instrument = "opentelemetry.instrumentation.auto_instrumentation:run"
opentelemetry-bootstrap = "opentelemetry.instrumentation.bootstrap:run"

[project.entry-points.opentelemetry_environment_variables]
instrumentation = "opentelemetry.instrumentation.environment_variables"

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Just a few outstanding questions.

@ocelotl
Copy link
Contributor

ocelotl commented Aug 23, 2022

Awesome, @ofek 😎

@lzchen lzchen closed this Aug 23, 2022
@lzchen lzchen reopened this Aug 23, 2022
@lzchen
Copy link
Contributor

lzchen commented Aug 23, 2022

@srikanthccv @ocelotl

Looks like Github is expecting for the Python3.6 test to run to pass status checks but we removed them from the build matrix in this PR. Should we remove Python3.6 from preventing builds passing?

@srikanthccv
Copy link
Member

Yes, I was wondering if there is way to continue run tests for 3.6 but that doesn't seem possible.

@ocelotl
Copy link
Contributor

ocelotl commented Aug 24, 2022

Yes, I was wondering if there is way to continue run tests for 3.6 but that doesn't seem possible.

Well, metrics is already stable, so we are ready to remove support for 3.6. I would prefer to first remove support for 3.6 and update our build system to use 3.7 in a separate PR just for clarity in our history, since dropping support for 3.6 is important for our users. I'll update #2360.

.github/workflows/test.yml Outdated Show resolved Hide resolved
@ofek
Copy link
Contributor Author

ofek commented Aug 24, 2022

This one? #2763

@ocelotl
Copy link
Contributor

ocelotl commented Aug 24, 2022

This one? #2763

Yes, I am working right now on making the test cases pass there.

@srikanthccv
Copy link
Member

@ofek The PR that dropped support for py36 is merged. Please resolve the conflicts and we can get this merged.

@ofek
Copy link
Contributor Author

ofek commented Aug 25, 2022

Done!

@srikanthccv srikanthccv dismissed ocelotl’s stale review August 25, 2022 18:04

The linked PR is merged now

@srikanthccv srikanthccv requested a review from ocelotl August 25, 2022 18:04
@ocelotl ocelotl merged commit a602192 into open-telemetry:main Aug 25, 2022
@ofek ofek deleted the modernize-metadata branch August 25, 2022 19:07
@ofek ofek changed the title Update package metadata Update package metadata opentelemetry-api Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants