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

fix(setuptools): use guess-next-dev schema #8796

Closed
wants to merge 3 commits into from

Conversation

tabgok
Copy link
Contributor

@tabgok tabgok commented Mar 27, 2024

A backport (#8367) changed our setuptools_scm schema from guess-next-dev to release-branch-semver for the 2.5 and 2.6 releases; but this change was never put into mainline. This change fixes the issue.

Prior to this change, setuptools_scm reported the version for patch releases as the next minor release. Afterwards, versions are patch releases (as expected).

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.
  • If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

A backport (#8367) changed our setuptools_scm schema from
`guess-next-dev` to `release-branch-semver` for the 2.5 and 2.6
releases; but this change was never put into mainline.  This change
fixes the issue.

Prior to this change, setuptools_scm reported the version for patch
releases as the next minor release.  Afterwards, versions are patch
releases (as expected).
@tabgok tabgok self-assigned this Mar 27, 2024
@tabgok tabgok marked this pull request as ready for review March 27, 2024 21:16
@tabgok tabgok requested review from a team as code owners March 27, 2024 21:16
@datadog-dd-trace-py-rkomorn
Copy link

Datadog Report

Branch report: teague.bick/DSMS-16/fix-schema
Commit report: 32ffb60
Test service: dd-trace-py

✅ 0 Failed, 171692 Passed, 1071 Skipped, 11h 26m 11.03s Total duration (21m 58.36s time saved)

@pr-commenter
Copy link

pr-commenter bot commented Mar 27, 2024

Benchmarks

Benchmark execution time: 2024-03-27 22:20:26

Comparing candidate commit 32ffb60 in PR branch teague.bick/DSMS-16/fix-schema with baseline commit be880c1 in branch main.

Found 5 performance improvements and 3 performance regressions! Performance is the same for 193 metrics, 9 unstable metrics.

scenario:flasksimple-appsec-telemetry

  • 🟥 execution_time [+220.949µs; +263.140µs] or [+3.527%; +4.201%]

scenario:flasksimple-tracer

  • 🟥 execution_time [+223.746µs; +270.052µs] or [+3.568%; +4.306%]

scenario:httppropagationextract-large_header_no_matches

  • 🟩 max_rss_usage [-744.847KB; -668.273KB] or [-3.406%; -3.056%]

scenario:httppropagationextract-medium_header_no_matches

  • 🟩 max_rss_usage [-761.444KB; -672.566KB] or [-3.482%; -3.076%]

scenario:httppropagationextract-none_propagation_style

  • 🟩 max_rss_usage [-908.887KB; -496.451KB] or [-4.152%; -2.268%]

scenario:httppropagationextract-wsgi_invalid_priority_header

  • 🟥 max_rss_usage [+435.174KB; +703.514KB] or [+2.057%; +3.325%]

scenario:httppropagationextract-wsgi_large_header_no_matches

  • 🟩 max_rss_usage [-808.458KB; -722.626KB] or [-3.691%; -3.299%]

scenario:httppropagationextract-wsgi_medium_header_no_matches

  • 🟩 max_rss_usage [-790.987KB; -715.931KB] or [-3.615%; -3.272%]

@@ -69,7 +69,7 @@ Homepage = "https://github.com/DataDog/dd-trace-py"
"Source Code" = "https://github.com/DataDog/dd-trace-py/"

[tool.setuptools_scm]
version_scheme = "release-branch-semver"
version_scheme = "guess-next-dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment above this property explaining why this needs to be release-branch-semver on the main branch

Copy link
Contributor

@christophe-papazian christophe-papazian left a comment

Choose a reason for hiding this comment

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

I am requesting this PR to be closed.

Explanations:

we have 2 possible policies for setup tools, none of them is ok for all cases.
Setuptools is looking back from current commit and looking for the first version tag in ancestor commit.
Then either it’s using that version with release-branch-semver or using the next minor dev version with guess-next-dev

Currently we need the next minor dev version to be able to enable new system tests for new “just merged in main” features. But we need the same version policy for all backports, otherwise we will enable tests in system tests for features merged in the next version.

So for now, we are using “guess-next-dev” in main and as soon as it’s needed we change it into “release-branch-semver” for minor version branches for all backports.

(This is a known problem that I already explained to a guild meeting, while sponsoring for one of the following solution, but this was never supported by the guild)

What are doing other big python projects on that problem ?

  • Do as we are doing (not really satisfying)
  • Use always “release-branch-semver” but add a new minor dev tag (for next unreleased minor version, like 4.15.0.dev) to the main branch on an empty commit after each creation of a new minor/major branch (like v4.14). We would need to integrate that step to our release todo list
  • Drop setuptools for version computation and have the version as constants in a file that would need to be updated for each release. Require more work but with less dependency, simpler build process and more control.

@tabgok
Copy link
Contributor Author

tabgok commented Mar 28, 2024

I created an issue here: #8801
And a specific PR for 2.7 here: #8800
And an explanatory comment here: #8802

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

Successfully merging this pull request may close these issues.

None yet

3 participants