Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Fix deprecated license syntax#549

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
sfmig wants to merge3 commits intomain
base:main
Choose a base branch
Loading
fromsmg/setuptools-license-deprecation

Conversation

@sfmig
Copy link
Contributor

@sfmigsfmig commentedApr 9, 2025•
edited
Loading

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
In CI we are getting two warnings re the syntax used to specify the license inpyproject.toml:

  • Thefirst one says:

    SetuptoolsDeprecationWarning:project.license as a TOML table is deprecated

    This followsPEP 639 and is also explained in thePython packing guide.

  • Thesecond one says:

    SetuptoolsDeprecationWarning: License classifiers are deprecated.

    This is also explained in the samePEP and in the packaging guidehere.

What does this PR do?
In pyproject.toml:

  • It changes the syntax used to specify the license
  • It adds alicense-files field
  • It removes the license as a classifier

In the pre-commit config file

  • It adds the necessary dependencies tocheck-manifest --no-build-isolation. The lower-bound forsetuptools needs to be set to 77, since that is the version when the new license syntax is supported (seehere).

Question

  • It feels a bit odd that we set different lower bounds for the setuptools version used in building and in checking the manifest file... Any thoughts about this?
  • What is the reasoning behind runningcheck-manifest in the precommits withno-build-isolation? From the check manifest repo, this option was introduced "so that check-manifest can succeed buildingpep517-based distributions without an internet connection". But: "With--no-build-isolation, you must preinstall thebuild-system.requires
    beforehand."
  • Shouldbuild-system.requires and the additional dependencies forcheck-manifest match?
  • If we agree to implement this, should we add it to other NIU repos and/or the cookiecutter?

References

\

How has this PR been tested?

  • pre-commits pass
  • check-manifest --no-build-isolation throws no warnings locally
  • No warnings are thrown in CI

Is this a breaking change?

\

Does this PR require an update to the documentation?

\

Checklist:

  • The code has been tested locally
  • [ n/a ] Tests have been added to cover all new functionality
  • [ n/a ] The documentation has been updated to reflect any changes
  • The code has been formatted withpre-commit

niksirbi reacted with thumbs up emoji
@codecov
Copy link

codecovbot commentedApr 9, 2025•
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base(529403f) to head(69583ea).

Additional details and impacted files
@@            Coverage Diff            @@##              main      #549   +/-   ##=========================================  Coverage   100.00%   100.00%           =========================================  Files           28        28             Lines         1555      1555           =========================================  Hits          1555      1555

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sfmigsfmig requested a review froma teamApril 9, 2025 13:40
@sfmigsfmig marked this pull request as ready for reviewApril 9, 2025 13:40
@sfmigsfmigforce-pushed thesmg/setuptools-license-deprecation branch frome801cba to448f46fCompareApril 24, 2025 10:04
Copy link
Member

@niksirbiniksirbi left a comment

Choose a reason for hiding this comment

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

Thanks@sfmig for taking the initiative to tackle this.

On your questions:

  • It feels a bit odd that we set different lower bounds for the setuptools version used in building and in checking the manifest file... Any thoughts about this?

I am worried about this to be honest, it might cause weird mismatches between local hooks and CI, which will be hard to debug.

  • What is the reasoning behind running check-manifest in the precommits with no-build-isolation? From the check manifest repo, this option was introduced "so that check-manifest can succeed buildingpep517-based distributions without an internet connection". But: "With --no-build-isolation, you must preinstall the build-system.requires beforehand."

I had totally forgotten the reason, but luckily git never forgets.
This was added to the cookiecutter by@lauraporta inthis PR, approved by me.

However, I now think we should re-examine and check whether it's truly necessary.

My preference would be to remove the--no-build-isolation arg, which means we will no longer needadditional_dependencies. This way we keep a single source of truth — the[build-system].requires inpyproject.toml. The added benefit is that each manifest check will happen in a fresh env, making sure we're not relying on package versions that happened to be locally present. However, I'm not 100% sure if I'm correctly understanding the docs on this.

  • Should build-system.requires and the additional dependencies for check-manifest match?

I'd say yes, if we follow my proposal above, and it works, we will only have the former (yay for DRY).

  • If we agree to implement this, should we add it to other NIU repos and/or the cookiecutter?

I would be in favour of that, unless we can't do that because of reasons mentionedhere.

@niksirbi
Copy link
Member

Beware that we might have a problem withpre-commit.ci, ifthis comment is still valid.

@sfmigsfmigforce-pushed thesmg/setuptools-license-deprecation branch from448f46f to69583eaCompareApril 28, 2025 20:08
@sonarqubecloud
Copy link

@lauraporta
Copy link
Member

Hey@sfmig, I faintly remember the bug related to--no-build-isolation. Reading again the previous PR and issues linked by@niksirbi, I have the fearcheck-manifest will try toapt install and fail as CIs are not authorised (as far as I am understanding) to install additional packages systemwide 🤔 We can try to remove it in a commit and see what happens to the CI 🤔

Fromcheck-manifest readme:

If you are running pre-commit without a network, you can utilize args: [--no-build-isolation] to prevent a pip install reaching out to PyPI. This makes python -m build ignore your build-system.requires, so you'll want to list them all in additional_dependencies.

So no need of additional pre-installations.

sfmig reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@niksirbiniksirbiniksirbi left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Status: 🚧 In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sfmig@niksirbi@lauraporta
[8]ページ先頭
©2009-2025 Movatter.jp