Skip to content

Conversation

@sfmig
Copy link
Contributor

@sfmig sfmig commented Apr 9, 2025

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 in pyproject.toml:

  • The first one says:

    SetuptoolsDeprecationWarning: project.license as a TOML table is deprecated

    This follows PEP 639 and is also explained in the Python packing guide.

  • The second one says:

    SetuptoolsDeprecationWarning: License classifiers are deprecated.

    This is also explained in the same PEP and in the packaging guide here.

What does this PR do?
In pyproject.toml:

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

In the pre-commit config file

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

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 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 building pep517-based distributions without an internet connection". But: "With --no-build-isolation, you must preinstall the build-system.requires
    beforehand."
  • Should build-system.requires and the additional dependencies for check-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 with pre-commit

@codecov
Copy link

codecov bot commented Apr 9, 2025

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.

@sfmig sfmig requested a review from a team April 9, 2025 13:40
@sfmig sfmig marked this pull request as ready for review April 9, 2025 13:40
@sfmig sfmig force-pushed the smg/setuptools-license-deprecation branch from e801cba to 448f46f Compare April 24, 2025 10:04
Copy link
Member

@niksirbi niksirbi 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 building pep517-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 in this 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 need additional_dependencies. This way we keep a single source of truth — the [build-system].requires in pyproject.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 mentioned here.

@niksirbi
Copy link
Member

Beware that we might have a problem with pre-commit.ci, if this comment is still valid.

@sfmig sfmig force-pushed the smg/setuptools-license-deprecation branch from 448f46f to 69583ea Compare April 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 fear check-manifest will try to apt 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 🤔

From check-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.

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

Labels

None yet

Projects

Status: 🚧 In Progress

Development

Successfully merging this pull request may close these issues.

4 participants