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

Align license_files globs with data_files globs #2720

Open
jaraco opened this issue Jul 5, 2021 · 5 comments
Open

Align license_files globs with data_files globs #2720

jaraco opened this issue Jul 5, 2021 · 5 comments

Comments

@jaraco
Copy link
Member

jaraco commented Jul 5, 2021

In #2712, Setuptools is considering adding glob support for declarative config for data_files. In that PR, I observed that license_files already has glob support, but handled in the dist and not the config. In this comment, option A proposes to align the license_files directive with this design, requiring users to use a glob: directive to indicate if a glob is present (and if wildcards are present, presume they're literals). To deal with the backward-incompatibility of this change, I propose the following transition for license_files:

  • Add support for glob: directive, and if present, expand the values when loading the config (same as data_files).
  • Deprecate globs without a glob: directive and warn if such a value expands due to globbing.
  • In the near future, O(months), remove support for globs without the directive.

If I recall, the license_files setting (and its default) is also entangled with license_file. Perhaps as part of this change, support for license_file could also be removed or at least decoupled from license_files, such that the default for license_files would be simply glob: LICEN[CS]E*, COPYING*, NOTICE*, AUTHORS*.

@cdce8p Since you contributed much (all?) of the license_files support, I'd like your opinion. Does this approach sound acceptable?

@cdce8p
Copy link
Contributor

cdce8p commented Jul 6, 2021

Thank you for bringing it to my attention. I have to admit that I needed to think about it a bit before writing this.

My goal for the original PR (#2620) was to make it easier to include license files by copying what wheel already does, i.e. using the patterns as globs: https://wheel.readthedocs.io/en/stable/user_guide.html#including-license-files-in-the-generated-wheel-file

It's true that adding the glob: directive would certainly make setuptools more consistent, it does however also introduces a challenge that I fear are not easily overcome:
wheel still depends on the license_files settings to compute which license files to include in the build distribution. Thus adding glob would make setuptools incompatible with wheel. It would require an update to wheel and would put additional requirements on which version of setuptools works with which version of wheel.

In the end it might be possible to do the deprecation, I'm just not sure if there is a true need for it. It should be fair to assume that most users either didn't add the license_files section, in which case the defaults are set by us, or know about it and expect it to work exactly like wheel i.e. the same output for the source and build distributions (at least with respect to the license files).

--
Why handled in dist not config
When I started to working on it I was new to setuptools. dist just had some prior work regarding the license handling and it made sense to add it there. Especially since originally it only modified the filelist. Support for the metadata item came later.

@darkvertex
Copy link
Contributor

What are your thoughts on this @jaraco?

It sounds like it might be too disruptive to add the glob: directive to license_files if we want to work the same way as wheel. Perhaps the choice of following "the wheel way" can be more explicitly explained in the setuptools documentation to prevent user confusion.

@jaraco
Copy link
Member Author

jaraco commented Jul 19, 2021

Sorry for not giving feedback sooner. I'm leaning that direction too. It definitely would be unfortunate to have different formats for the same concept in the same tool and preferable to converge across tools.

@darkvertex Would you be willing to adapt the PR to implement implicit glob support?

@darkvertex
Copy link
Contributor

@darkvertex Would you be willing to adapt the PR to implement implicit glob support?

Sure, no problem. I'll get back to you.

@darkvertex
Copy link
Contributor

@jaraco Updated my PR: #2712 👍

Since we're going for "implicit" globbing, I made a choice to support globby-like filenames literally should they exist, so if someone made such a file and is expecting it to match the real file on disk instead of expanding the pattern, they will get the expected result (I think!) -- At the same time, it's a bit of an edge case, but.. 🤷‍♂️

If you disagree with this decision we can take it out but I think it's what a user would expect.

You can see my updated unit test for an example using a file with a question mark and having it match the file correctly as a literal value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants