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

Add pypi name validator #100646

Closed
wants to merge 8 commits into from
Closed

Add pypi name validator #100646

wants to merge 8 commits into from

Conversation

edenhaus
Copy link
Contributor

@edenhaus edenhaus commented Sep 20, 2023

Breaking change

Proposed change

Extract all changed requirements from the git diff, and for each, we request the project info from Pypi to verify that the name matches case sensitive. If not, an error will be logged.
The following files will be checked: ["requirements*.txt", "homeassistant/package_constraints.txt"]

The script supports different modes (specified by --mode):

  • diff-branch -> get requirements from the diff with the base dev branch (uses git merge-base to identify it)
  • diff-staged -> get requirements from git diff --staged and used when running the check with pre-commit
  • all -> get all requirements from all files searched with the above pattern list

An example of a fail pre-commit looks like:
image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant home-assistant bot added cla-signed small-pr PRs with less than 30 lines. labels Sep 20, 2023
@edenhaus edenhaus added the core label Sep 20, 2023
@edenhaus edenhaus mentioned this pull request Sep 20, 2023
20 tasks
@edenhaus
Copy link
Contributor Author

Not sure the reason / history for this. pypa/pip#12038 might be relevant since its now fixed.

Originally posted by @bdraco in #100648 (comment)

@bdraco As we still have some requirements not spelled case-sensitive correctly, I have added this check. The reason is that we want to improve the build speed. In #93883 you mention that the names must match. Is this still correct?

@bdraco
Copy link
Member

bdraco commented Sep 20, 2023

The names shouldn't have to match anymore now that pip fixed the bug

Once we have an index https://github.com/bdraco/index-503 on the wheels server, all the wheel builds and builds should be able use it and be much faster

For clarity, we shouldn't have to change anything in HA core except to use the pip cli options described at the repo above, now that pip made the name matching work correctly

@bdraco
Copy link
Member

bdraco commented Sep 20, 2023

Feel free to ping me on discord if you have any questions. I'm about to jump a short flight though so might be a few hours before I can respond

@edenhaus edenhaus mentioned this pull request Sep 21, 2023
20 tasks
@edenhaus
Copy link
Contributor Author

We should not require anymore that the names be spelled correctly, but until we have tested the index with our wheel server, this PR will be left open as draft.

@frenck
Copy link
Member

frenck commented Sep 21, 2023

I think we should keep around the script, but remove the CI part.

That way, we can merge it and have access to the script whenever we like.

../Frenck

@edenhaus edenhaus marked this pull request as ready for review September 21, 2023 09:52
@edenhaus edenhaus requested review from frenck and a team September 21, 2023 09:53
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Looks it does still complain about this

@bdraco
Copy link
Member

bdraco commented Oct 1, 2023

Discarding https://wheels.home-assistant.io/musllinux-index/aiohttp_cors-0.7.0-py3-none-any.whl#sha256=0451ba59fdf6909d0e2cd21e4c0a43752bc0703d33fc78ae94d9d9321710193e (from https://wheels.home-assistant.io/musllinux-index/aiohttp-cors/): Requested aiohttp_cors==0.7.0 from https://wheels.home-assistant.io/musllinux-index/aiohttp_cors-0.7.0-py3-none-any.whl#sha256=0451ba59fdf6909d0e2cd21e4c0a43752bc0703d33fc78ae94d9d9321710193e (from -r /tmp/wheels_requirement.txt (line 276)) has inconsistent Name: expected 'aiohttp_cors', but metadata has 'aiohttp-cors'

@bdraco
Copy link
Member

bdraco commented Oct 2, 2023

I wonder if the wheel builder is using an old version of pip

@frenck
Copy link
Member

frenck commented Oct 6, 2023

It actually is using an old version.

@frenck
Copy link
Member

frenck commented Oct 9, 2023

These errors/warnings are now gone. So, what does that mean for this PR now?

../Frenck

@edenhaus
Copy link
Contributor Author

edenhaus commented Oct 9, 2023

Closing as the PR is not required anymore as pip handle all the names correctly

@edenhaus edenhaus closed this Oct 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2023
@edenhaus edenhaus deleted the pip-name-checker branch August 28, 2024 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed core small-pr PRs with less than 30 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants