Skip to content

Replace auto-install of setuptools_scm with error in helper scripts #11441

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 3, 2024
Merged

Replace auto-install of setuptools_scm with error in helper scripts #11441

merged 3 commits into from
Sep 3, 2024

Conversation

silv-io
Copy link
Member

@silv-io silv-io commented Sep 2, 2024 •
edited
Loading

Motivation

With the introduction of setuptools_scm our helper scripts started to depend on it. First, we added an automatic install of this dependency. However, unknowing users of those scripts might then install those dependencies into their global python environment which they might not want.
Decoupling the installation from the script execution will solve this.

Changes

  • Change check for installation of setuptools_scm to use pip show, so that both setuptools-scm and setuptools_scm can be detected (at some version, they switched the naming)
  • Display an ERROR when the package is missing instead of quietly installing it in the active environment
  • Change the workflows that use the helper script to install the newest versions of setuptools and setuptools-scm to avoid version incompatibilities.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2024 •
edited
Loading

Helper Script Tests

28 tests   28 ✅  0s ⏱️
 2 suites   0 💤
 1 files     0 ❌

Results for commit 01200b5.

♻️ This comment has been updated with latest results.

@silv-io silv-io force-pushed the fix-setuptools-scm-dependency branch from c7e04c4 to ec21b76 Compare September 2, 2024 08:11
@silv-io silv-io added the semver: patch Non-breaking changes which can be included in patch releases label Sep 2, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2024 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   4m 3s ⏱️ +37s
420 tests ±0  368 ✅ ±0   52 💤 ±0  0 ❌ ±0 
840 runs  ±0  736 ✅ ±0  104 💤 ±0  0 ❌ ±0 

Results for commit 01200b5. ± Comparison against base commit fcd110d.

♻️ This comment has been updated with latest results.

@silv-io silv-io force-pushed the fix-setuptools-scm-dependency branch 4 times, most recently from 204e3fb to febde81 Compare September 2, 2024 09:58
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 35m 53s ⏱️ - 1m 10s
3 418 tests ±0  3 020 ✅ ±0  398 💤 ±0  0 ❌ ±0 
3 420 runs  ±0  3 020 ✅ ±0  400 💤 ±0  0 ❌ ±0 

Results for commit 01200b5. ± Comparison against base commit fcd110d.

♻️ This comment has been updated with latest results.

@silv-io silv-io marked this pull request as ready for review September 2, 2024 11:11
@silv-io silv-io force-pushed the fix-setuptools-scm-dependency branch from febde81 to 5e94f13 Compare September 2, 2024 12:17
alexrashed
alexrashed reviewed Sep 2, 2024
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks a lot for jumping on this, and explicitly following up on @dfangl's comment on #11355. 🦸🏽
The changes are looking good, but I think it would really be good to make sure that we keep testing the scripts, now that we have a proper testing infra in place.

@alexrashed alexrashed mentioned this pull request Sep 2, 2024
Merged
9 tasks
@silv-io silv-io requested a review from alexrashed September 2, 2024 15:30
alexrashed
alexrashed approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for addressing the comments! This is looking great! 💯

@silv-io silv-io merged commit e14e0c0 into master Sep 3, 2024
41 of 42 checks passed
@silv-io silv-io deleted the fix-setuptools-scm-dependency branch September 3, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

@thrau thrau Awaiting requested review from thrau

@dfangl dfangl Awaiting requested review from dfangl

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@silv-io @alexrashed