Implement #1681 (globbing support for [options.data_files])

[darkvertex]

Summary of changes

Hello! 👋 First time contributor here.

This change (which closes #1681) introduces an optional new glob: directive just for values of [options.data_files] which will expand the given patterns to the corresponding relative paths.

It is also possible to do a mix of some regular strings and some using the new directive in the same dict. (See new unittest for an example.)

I took the liberty of updating docs/userguide/declarative_config.rst to mention the new directive. Please advise if it should be noted elsewhere in the documentation.

I tried to stay within the style of the file and added a new unittest for this new feature. Let me know what you think or if you wish to reword or adjust anything.

Pull Request Checklist

• Changes have tests
• News fragment added in changelog.d/.
  (See documentation for details)

[jaraco]

This work is really good... good enough to merge after fixing a couple of nitpicks (done).

My only hesitation is that license_files, which was introduced recently, also accepts globs, but using a different syntax (no glob: prefix) and relies on postprocessing to expand the globs.

At some point, it would probably be worthwhile to harmonize those two mechanisms (if that makes sense). And of course, it might prove easiest to harmonize the two before this setting adopts the glob feature and users come to depend on it.

@darkvertex Would you be willing to analyze these two settings and make a recommendation on how you'd like to proceed?

[darkvertex]

@jaraco Hi! Thanks for the review and doing the little adjustments.

This work is really good... good enough to merge after fixing a couple of nitpicks (done).

Thanks! :)

---

My only hesitation is that license_files, which was introduced recently, also accepts globs, but using a different syntax (no glob: prefix) and relies on postprocessing to expand the globs.

At some point, it would probably be worthwhile to harmonize those two mechanisms (if that makes sense). And of course, it might prove easiest to harmonize the two before this setting adopts the glob feature and users come to depend on it.

@darkvertex Would you be willing to analyze these two settings and make a recommendation on how you'd like to proceed?

@jaraco If I understand correctly, the license_files with glob support is already released to the public, right? Doesn't that make it too late to add the notion of the glob: prefix/directive to it and telling people to explicitly use that when they want globbing?

Or did you mean to harmonize as in make both functions reuse the same glob expansion logic?

I see two avenues of going about this:

---

A) If we want to make license_files support the glob: directive but not expand patterns by default -- which it does currently expand, as I understand it, so it would be a breaking change but I think it's the most logical/harmonious way to bring both together -- then it'd be super easy to just change line 557 in config.py:
https://github.com/darkvertex/setuptools/blob/data_files_glob_directive/setuptools/config.py#L557
from 'license_files': parse_list, to 'license_files': parse_list_glob, and do away with the _expand_patterns( stuff in line 583 of dist.py:

setuptools/setuptools/dist.py

Line 583 in 0aa3576

def _finalize_license_files(self):

but keeping the logic about searching for some default patterns (patterns = ('LICEN[CS]E*', 'COPYING*', 'NOTICE*', 'AUTHORS*')) when no values are declared whatsoever.

---

B) If you wanted to simply bring some of the core glob logic together to reduce repetition / duplication, not changing any behaviour of license_files, I could move _expand_patterns():

setuptools/setuptools/dist.py

Line 602 in 0aa3576

def _expand_patterns(patterns):

into config.py and introduce the os.path.relpath() bit I had in my expansion -- which does not seem to break any related tests; I tried it:

    def _expand_patterns(patterns):
        """
        >>> list(Distribution._expand_patterns(['LICENSE']))
        ['LICENSE']
        >>> list(Distribution._expand_patterns(['setup.cfg', 'LIC*']))
        ['setup.cfg', 'LICENSE']
        """
        return (
            os.path.relpath(path, os.getcwd())
            for pattern in patterns
            for path in sorted(iglob(pattern))
            if not path.endswith('~')
            and os.path.isfile(path)
        )

then I could use it in line 280:
https://github.com/darkvertex/setuptools/blob/data_files_glob_directive/setuptools/config.py#L280
where it says:

                value = sorted(
                    os.path.relpath(path, os.getcwd()) for path in iglob(value))

and edit it to reuse the function:

                value = sorted(cls._expand_patterns([value]))

and then make sure it still gets used in dist.py's _finalize_license_files() func (after importing it at the top of the file, of course.)

There's already a few things being imported from config.py in dist.py, so it would not be wise to move that logic completely to dist.py as that could cause a circular import situation.

---

How would you guys like to proceed?

Personally I prefer option A: make license_files use the new glob: directive and break its globbing behaviour from happening by default when not empty, but I totally understand if we want to avoid pissing people off since it seems to be a released feature (though then again, it is very recent so maybe it wouldn't piss off that many! ha)

[eli-schwartz]

We could also slightly reverse tracks and decide to expand globs by default in data_files. I discussed the tradeoffs in #1681 (comment)

[jaraco]

If you wanted to simply bring some of the core glob logic together to reduce repetition / duplication

I wasn't thinking about avoiding code duplication, so thanks for raising that possibility. I do, however, want to focus primarily on the user interface first and then address implementation details subsequently.

I'm liking option A, and I do think that's the best approach.

In that case, it doesn't particularly affect this PR, so I'll likely merge this soon.

[darkvertex]

As requested by @jaraco in his comment I've adapted the code such that globbing is implicit.

Looks like someone needs to approve the Github workflows again for me since I'm a first-time contributor. Can someone help?

[darkvertex]

I fixed the line-too-long flake8 warning. Sorry about that.

Can someone re-run the tests please?

[darkvertex]

No rush, but just sharing that I'll be AFK on vacation for three weeks starting next week. So if anyone wants any further little edits from me, now is the time to ask for them (or else see ya late August.)

[jaraco]

Thanks for the patience and extra re-work.

[darkvertex]

No problem! Thanks for reviewing and merging.

Looking forward to cleaning up my setup.py files that only existed to do some globbing in some of my pip packages at work. 😀

[darkvertex]

@jaraco I see you tried to create a release but Github Actions workflow failed due to a test failure.

Thought you'd like to know that it would appear that it's an issue with sphinx being incompatible with Python 3.10 and apparently not an issue with the code of setuptools:
sphinx-doc/sphinx#9505 (comment)

I also found this fix for the line causing the type error seen in this repo's failed build:
sphinx-doc/sphinx@8b2031c
...but it doesn't seem to be in the latest stable sphinx release yet.