Work around setuptools 63 deprecations #502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Ok, there is one problem: dotfiles in subdirectories of docs/ or tests/ are kept as well. |
|
Aaand I was able to solve that one by adding: this gets rid of all (Don't ask me how I figured that out... resp. how many combinations I tried...) |
|
Hmm, while that works, I noticed another problem: subdirectories further down in the tree that are called |
|
The thing is that setuptools's |
|
I was able to work around these problems by avoiding wildcards if possible, but some files are still removed from the wheel, for example: while these are files we actually don't want to have in the wheel, this could have also hit other files like |
|
Maybe it would be easier to simply remove all dotfiles everywhere. You can find all 474 dotfiles (and dot directories) in a hypothetical Ansible 8.0.0 release below.
When restricting to dotfiles (and dot directories) outside of the collection root, there are 206 ones:
|
|
Thanks for working on this!
I'd like to do that, but I worry it'll break things. For example, I have a role that installs dotfiles on my systems and all of the FWIW, we have a hard coded list of dotfiles that we always in remove in the Fedora package (https://src.fedoraproject.org/rpms/ansible/blob/rawhide/f/ansible.spec#_101). I tried to remove this when we started excluding tests and dotfiles from the wheels upstream, but as you showed, there's still a fair amount of dotfiles in other directories. Maybe we could use this list to exclude dotfiles from other directories (i.e. not the top level where we exclude everything) in the setup.py. |
gotmax23
reviewed
Mar 31, 2023
|
The latest change adds back I generated a hypothetical 8.0.0 with both the I guess the problem with the |
|
Ok, this finally works! It produces almost the same files that are included as the version in the So from my POV, this is now ready for review! (Since Ansible 8.0.0a1 is due soon, we shouldn't wait for too long if we want this to be in 8.0.0.) |
|
Thanks! I'll take a closer look later. |
|
I noticed that |
gotmax23
reviewed
Apr 7, 2023
Comment on lines
592
to
604
| # Collect collection namespaces and collection root subdirectories | ||
| collection_namespaces: dict[str, list[str]] = defaultdict(list) | ||
| collection_directories: dict[str, list[str]] = {} | ||
| for collection in dependency_data.deps: | ||
| namespace, name = collection.split('.', 1) | ||
| collection_namespaces[namespace].append(name) | ||
| collection_path = os.path.join(ansible_collections_dir, namespace, name) | ||
| collection_directories[collection] = [ | ||
| file for file in os.listdir(collection_path) | ||
| if file not in ('tests', 'docs') | ||
| and not file.startswith('.') | ||
| and os.path.isdir(os.path.join(collection_path, file)) | ||
| ] |
There was a problem hiding this comment.
Suggestion: you could try moving this to a function that returns a tuple of collection_names, collection_namespaces, collection_directories to reduce the mccabe complexity.
There was a problem hiding this comment.
I moved a slightly different block out: f63694b.
I'd be happy to help cut the release and learn the process, but I don't think I have the permissions for that. |
|
Do you have permissions to push to But we can try to do it together; I'm not sure whether I'll have time later today but tomorrow should be ok. |
|
I think I can push to |
gotmax23
approved these changes
Apr 7, 2023
Well, looks like your invite expired, since you didn't accept it. I'll try to re-send it. |
|
@gotmax23 thanks for testing, reviewing, and merging, and of course also working on this! :) |
Doh, it seems I completely missed those emails :). I've accepted the new ones. Thanks. I plan to release 0.55.0 tomorrow morning (it's midnight here now). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #433.
Closes #434.
This uses a modified (#433 (comment), #433 (comment)) version of @gotmax23's proposal in #433 (comment).
This only uses the new approach from Ansible 8 on, to avoid suddenly breaking Ansible 7.5.0+.