Fix deprecated license syntax

[sfmig]

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (e6420e9) to head (7e6094f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #549   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         2111      2111           
=========================================
  Hits          2111      2111           

☔ 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.

[niksirbi]

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]

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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[lauraporta]

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.

[niksirbi]

I did a deeper dive into your questions @sfmig, and this is my current take (updated from my previous views):

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?

It is technically "fine" for these to differ, it just means that the pre-commit environment is stricter than our declared build system. However, I don't like it for maintainability reasons. More importantly, as we're now switching to the new licence syntax in pyproject.toml, which requires setuptools>=77, I'm not even sure if our package can be built with older versions. I recommend setting the lower bounds to 77 everywhere.

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."

This is my understanding of the trade-offs posed by using no-build-isolation:

Usually, check-manifest calls python -m build. By default, build creates a fresh, temporary virtual environment and installs everything in build-system.requires from the internet. Creating that environment every time you run a pre-commit hook may be slow, and it may fail completely if we lose internet (e.g. whilst coding on a train/plane).

By using --no-build-isolation we gain speed and robustness to network losses, but lose the 'purity' of the check. If we forget to add a dependency to additional_dependencies, the check might fail or give a false positive because the environment isn't a perfect replica. So with --no-build-isolation, we accept the responsibility to keep the hook's additional_dependencies aligned with build-system.requires. I think that is a price worth paying, especially given that this setup has been working for us so far.

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

My understanding is that they don't have to match exactly, but they should be compatible.

check-manifest --no-build-isolation needs a build environment that can successfully build our sdist/wheel, so its additional_dependencies should still satisfy build-system.requires.

To be extra sure of the compatibility, we should also constrain setuptools_scm[toml]>=6.2 in pre-commit.

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

Yes, this setup is not unique to movement. Whatever the outcome, it should apply to all of our repositories.

My recommendations

1. Set setuptools>=77 in both build-system.requires and pre-commit-config.yaml. This seems to me as the most future-proof option.
2. Keep using --no-build-isolation but constrain setuptools_scm[toml]>=6.2 in the pre-commit config to match pyproject.toml.
3. Propagate these changes to the cookiecutter.

Note

This PR will have to be rebased to main as it is severely out-of-sync.

[niksirbi]

I've just rebased this PR to main and resolved merge conflicts.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud