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

Since setuptools 28.5.0, a "global-exclude .pyc" no longer matches .pyc files #849

Closed
rbarrois opened this Issue Nov 15, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@rbarrois
Copy link

rbarrois commented Nov 15, 2016

Since commit bb45468, if a project's MANIFEST.in contains global-exclude .py[co], the archive generated with python setup.py sdist will still contain all .pyc / .pyo files.

The test below shows that this line worked in setuptools 28.4.0, and broke in setuptools 28.5.0.

I'm not sure whether this can be considered a bug: the previous implementation looked up patterns without any anchoring, whereas the new one forces users to explicitly include wildcards in their MANIFEST.in.
However, I think this change should have been mentioned in the version Changelog.

diff --cc setuptools/tests/test_manifest.py
index 6360270,ef4beda..0000000
--- a/setuptools/tests/test_manifest.py
+++ b/setuptools/tests/test_manifest.py
@@@ -425,12 -464,22 +425,19 @@@ class TestFileListTest(TempDirTestCase)
          assert file_list.files == ['b.txt']
          self.assertWarnings()
  
+         file_list = FileList()
+         file_list.files = ['a.py', 'b.txt', l('d/c.pyc'), 'e.pyo']
+         file_list.process_template_line('global-exclude .py[co]')
+         file_list.sort()
+         assert file_list.files == ['a.py', 'b.txt']
+         self.assertNoWarnings()
+ 
          # recursive-include
@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Nov 19, 2016

Thanks @rbarrois for tracing the issue and providing a unit test.

I agree this looks like an unintentional regression.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Nov 19, 2016

According to the changelog, #764 is implicated in this change. Since the unit tests didn't capture this behavior before, it's no surprise that it was missed during the re-write.

@timheap How difficult would it be to restore this expectation?

timheap added a commit to timheap/setuptools that referenced this issue Nov 20, 2016

Fix pypa#849 global-exclude globbing
After pypa#764, `global-exclude .pyc` no longer excluded `.pyc` files.
This fixes that regression, and adds a test for this behaviour.
@timheap

This comment has been minimized.

Copy link
Contributor

timheap commented Nov 20, 2016

@jaraco It is simple to restore this behaviour for global-include and global-exclude. I've done so over in #853. Sorry for breaking your build, @rbarrois!

However, I will argue that the old behaviour is unexpected and not desired. In my opinion, a note in the Changelog about this breaking change should be included, rather than restoring the old behaviour. The new behaviour behaves much more like globbing does in other environments (e.g. a bash shell), and is less surprising. With the old behaviour, a pattern like global-exclude bar.py would exclude bar.py, but also surprisingly foo_bar.py, and there is no simple way of stopping this from happening. Under the new behaviour, global-exclude bar.py would only exclude bar.py, and not foo_bar.py. @rbarrois has already found the simple work around to restore the old behaviour by explicitly globbing the match like global-exclude *.py[co].

@rbarrois

This comment has been minimized.

Copy link
Author

rbarrois commented Nov 21, 2016

@timheap thanks! I do agree with you: I wouldn't consider "properly anchoring patterns" as a bug, but rather as a proper bugfix. I'm totally in favor of keeping the fixed behavior, and simply documenting "a longstanding bug has been fixed in passing while cleaning up this code — please fix your manifests.".

@jaraco jaraco closed this in #853 Dec 3, 2016

jaraco added a commit that referenced this issue Dec 3, 2016

timheap added a commit to timheap/setuptools that referenced this issue Dec 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.