Read the existing MANIFEST.in file for files to ignore. #19

mauritsvanrees · 2013-10-04T21:45:29Z

The existing MANIFEST.in can have been setup to ignore some files or complete directories. check-manifest should not complain that those are in version control but not in the sdist.

If MANIFEST.in exists, we read it and add relevant entries to the IGNORE definition. To properly support the exclude directive I needed to add an IGNORE_REGEXPS definition.

Tests pass on the two supported pythons that I have: 2.7 and 3.3.

Use this to support ignoring patterns that are in MANIFEST.in with the 'exclude' keyword.

coveralls · 2013-10-04T21:46:49Z

Coverage increased (+2.2%) when pulling 76e42db on mauritsvanrees:maurits-read-manifest into c8e785a on mgedmin:master.

mauritsvanrees · 2013-10-04T21:57:19Z

Strange: the above comment by coveralls says the coverage increased, but I got a mail from coveralls and that said:

"Coverage remained the same when pulling 76e42db on mauritsvanrees:maurits-read-manifest into c8e785a on mgedmin:master."

The percentage is 78 in both cases. I had this difference with a previous pull request too. Oh well.

mgedmin · 2013-10-06T14:02:43Z

check_manifest.py

+    # XXX modifies global state, which is kind of evil
+    if not os.path.isfile('MANIFEST.in'):
+        return
+    contents = open('MANIFEST.in').read()


Please use the with statement to avoid ResourceWarnings on Python 3.x and leaking file descriptors on PyPy.

mgedmin · 2013-10-06T14:11:02Z

Thank you for the pull request.

I'd like to see some examples of projects that have files contained in the version control system that must be excluded from source distributions. Can you satisfy my curiosity?

This avoids ResourceWarnings on Python 3.x and leaking file descriptors on PyPy.

'split' already ignores leading and trailing whitespace.

Remove leading whitespace programmatically with textwrap.dedent.

coveralls · 2013-10-07T08:36:44Z

Coverage increased (+2.33%) when pulling 3a5afc6 on mauritsvanrees:maurits-read-manifest into c8e785a on mgedmin:master.

mauritsvanrees · 2013-10-07T08:51:34Z

Thanks for your detailed feedback on the code. I have pushed the improvements.

The example that caused me to have a look at this, is plone.app.contenttypes. When I run check-manifest in a clone, I get this output:

$ check-manifest 
listing source files under version control: 169 files and directories
copying source files to a temporary directory
building an sdist: plone.app.contenttypes-1.0dev.tar.gz: 155 files and directories
files in version control do not match the sdist!
missing from sdist:
  .travis.yml
  bootstrap.py
  buildout.cfg
  dev.cfg
  headers.sh
  jenkins.cfg
  sphinx.cfg
  templates
  templates/code-analysis.sh
  test-plone-4.1.x.cfg
  test-plone-4.2.x.cfg
  test-plone-4.3.x-ecosystem.cfg
  test-plone-4.3.x.cfg
  travis.cfg
suggested MANIFEST.in rules:
  include *.py
  include *.sh
  include .travis.yml
  include buildout.cfg
  include dev.cfg
  include jenkins.cfg
  include sphinx.cfg
  include test-plone-4.1.x.cfg
  include test-plone-4.2.x.cfg
  include test-plone-4.3.x-ecosystem.cfg
  include test-plone-4.3.x.cfg
  include travis.cfg
  recursive-include templates *.sh

There is a MANIFEST.in already. It has no relevant excludes yet, but with the code in this pull request I could add these lines to the manifest and then check-manifest no longer complains:

exclude bootstrap.py *.cfg *.yml *.sh
prune templates

An argument against this would be that these are files that are not normally included by Python, so it may be better to tell check-manifest to ignore these by adding a few lines to setup.cfg. But someone may have installed the setuptools-git package and then these files would be included.

Perhaps a better example is that some packages prune the tests directory, for example Products.TinyMCE. Without the code from this pull request, check-manifest complains:

missing from sdist:
  Products/TinyMCE/tests
  Products/TinyMCE/tests/__init__.py
  Products/TinyMCE/tests/base.py
  ...

mgedmin · 2013-10-07T14:16:34Z

check_manifest.py

+    # Regular expressions for filename to ignore.  This is useful for
+    # filename patterns where the '*' part must not search in
+    # directories.
+    ]


The closing ] should be unindented.

I believe pep8 warns about this, but even if it doesn't, let's keep it consistent with the rest of the file.

mgedmin · 2013-10-07T14:30:09Z

As for your project examples, I think all those are source files that should be included in source distributions. Tests are part of the source code! Build configuration is something you need if you want to work on a package. Even .travis.yml, while functionally useless in a tarball, can act as documentation for people telling them how to run the tests.

It seems tabs are accepted in the manifest. We now split each line to check for a command (exclude, prune, etc) and arguments.

coveralls · 2013-10-08T08:33:20Z

Coverage increased (+3.15%) when pulling 77fe8dc on mauritsvanrees:maurits-read-manifest into c8e785a on mgedmin:master.

mauritsvanrees · 2013-10-08T08:49:11Z

I am done with the fixes.

I actually never exclude the tests in my packages, but I can imagine that in some projects people think they bloat the package too much. And lots of source code packages have the same bootstrap.py and it does help much to distribute that. Also, it will not actually be available once you have installed it as an egg: it ends up in the source distribution but not in the egg.

Anyway, the pull request does not advocate excluding or pruning stuff, but only helps in not letting check-manifest complain about missing those files, but I think you got that point.

mgedmin · 2013-10-08T19:31:33Z

check_manifest.py

+        elif cmd == 'global-exclude':
+            ignore.extend(rest.split())
+        elif cmd == 'recursive-exclude':
+            dirname, patterns = rest.split(None, 1)


I think this can raise ValueError if you've got a malformed MANIFEST.in with recursive-exclude dir but no patterns.

True. I was thinking: if this is wrong then python itself will complain when trying to use the manifest.

Well, python prints a warning but happily continues:

warning: manifest_maker: MANIFEST.in, line 6: 'recursive-exclude' expects <dir> <pattern1> <pattern2> ...

I have changed to code to print the same warning (without line number) when we get a ValueError.

Oh, I have added a test for this. This means that running the tests will print a warning. Not sure if that is a thing to avoid.

I prefer tests to pass without warnings or in fact any extra output to stdout/stderr. This should be easy to arrange by adding a

with warnings.catch_warnings(): self.assertEqual(function_that_causes_the_warning(...), ...)

We could even test the spelling of the warning with

with warnings.catch_warnings(record=True) as w: self.assertEqual(function_that_causes_the_warning(...), ...) self.assertEqual(w, ['...'])

but I'm not convinced it's worth the effort.

coveralls · 2013-10-09T08:55:53Z

Coverage increased (+3.48%) when pulling b62f61a on mauritsvanrees:maurits-read-manifest into c8e785a on mgedmin:master.

mauritsvanrees · 2013-10-09T08:57:37Z

Thanks for your continuing constructive feedback. I have pushed the commits based on your remarks. I am now running tox with all four supported pythons and the tests all pass.

Should you finally decide not to accept this pull request, I have at least learned a lot about MANIFEST.in. ;-)

mgedmin · 2013-10-09T19:10:08Z

Incidentally I came up with a really good use-case for the 'exclude' feature: buildout. It has two packages in one source tree (zc.buildout and zc.recipe.egg), with separate setup.py files and everything.

Ironically, its MANIFEST.in does not use the exclude feature.

mgedmin · 2013-10-09T19:26:36Z

I keep wondering about one thing: if one has a setuptools version control system plugin that tells it to include some file, but the MANIFEST in tells it to exclude the same file, what will setuptools do?

I hope it'll exclude that file, in which case I have no further reservations for merging this PR. Well, except those two small fixes I asked about: shutting up warnings and handling recursive-exclude dir prefix*. And I worry a bit that all those string.rstrip(os.path.sep) will make Windows compatibility a pain. And I worry about blindly using a glob pattern as a regexp, after escaping just two of the special characters (. and *), but ignoring the rest (e.g. ? is special in glob but has a completely different meaning in regexps; I don't expect + or | or (/) to show up in filename patterns...).

Eh, we can deal with those issues when the bugs get filed. I'll wait for updates for warnings and prefix*, then merge this. Or if you're tired of complying with my unreasonable demands, I can merge and do the fixes myself.

coveralls · 2013-10-10T10:49:10Z

Coverage increased (+4.25%) when pulling f83b42d on mauritsvanrees:maurits-read-manifest into c8e785a on mgedmin:master.

mauritsvanrees · 2013-10-10T10:57:38Z

I notice one nasty thing that seems really unexpected in MANIFEST.in. If for example in plone.app.contenttypes I add this line I expect that the sdist will exclude upgrades.py and browser/utils.py and a few more like that:

recursive-exclude plone/app/contenttypes u*

That is correct, but a file like subscribers.py is also excluded, simply because there is a u followed by some more characters. That there is an s in front is disregarded. This has got nothing to do with check-manifest, this is just what happens when I check the result of python2.7 setup.py sdist.

With the current status of this pull request, check-manifest complains that subscribers.py and some more files are not in the sdist. This means that check-manifest does not parse the MANIFEST.in correctly, but I am inclined to say that it is good to warn here, because for most people this will be unexpected.

mgedmin · 2013-10-10T11:10:24Z

headdesk distutils

Ignore warnings in one known spot in the tests. This keeps the test output clean.

coveralls · 2013-10-10T11:13:14Z

Coverage increased (+4.31%) when pulling 7538143 on mauritsvanrees:maurits-read-manifest into c8e785a on mgedmin:master.

mauritsvanrees · 2013-10-10T11:27:17Z

Quoting you: "I keep wondering about one thing: if one has a setuptools version control system plugin that tells it to include some file, but the MANIFEST in tells it to exclude the same file, what will setuptools do?"

It excludes them. If I use a clean python without setuptools plugins and use it to create an sdist of a plone package without MANIFEST.in, it will not contain for example the zcml files. If I install setuptools-git and recreate the sdist, the zcml files are included. If I add a line in MANIFEST.in to exclude zcml files, they are again excluded.

The warnings are ignored now in the tests, which means I had to change the existing warning function, because it simply printed to stderr. Note that the warnings module by default prints to stderr too.

I'm not tired of unreasonable or reasonable demands yet. ;-)

mgedmin · 2013-10-10T11:45:21Z

Gah! Sorry, I completely forgot that there was an existing warning() function, and assumed you used the warnings module.

Read the existing MANIFEST.in file for files to ignore.

mauritsvanrees · 2013-10-10T15:04:26Z

0.17 is released I see. Thanks!

myint · 2013-10-11T03:01:49Z

I guess this accomplishes what I originally wanted in #10. Thanks!

myint · 2013-10-11T04:43:21Z

By the way, if you ever want to get rid of the --ignore option I added, that would be fine with me.

mauritsvanrees added 4 commits October 4, 2013 22:12

Read the existing MANIFEST.in file for files to ignore.

eee444f

Add list for ignoring regular expressions.

9523976

Use this to support ignoring patterns that are in MANIFEST.in with the 'exclude' keyword.

Test strip_sdist_extras with a MANIFEST.in file.

0903222

changelog

76e42db

mgedmin reviewed Oct 6, 2013
View reviewed changes

mauritsvanrees added 4 commits October 7, 2013 10:13

Use the 'with' statement for opening files.

f8a593f

This avoids ResourceWarnings on Python 3.x and leaking file descriptors on PyPy.

Remove reduntant 'strip'.

4fc8b69

'split' already ignores leading and trailing whitespace.

Do not make a dot into a magical wildcard character.

6560439

Keep the indentation visually clear in the test.

3a5afc6

Remove leading whitespace programmatically with textwrap.dedent.

mgedmin reviewed Oct 7, 2013
View reviewed changes

mauritsvanrees added 4 commits October 8, 2013 09:57

strip not needed in combination with split.

552e886

Better whitespace checking when parsing a MANIFEST.in line.

d4ef7d5

It seems tabs are accepted in the manifest. We now split each line to check for a command (exclude, prune, etc) and arguments.

Make sure prune gets no path separator.

c68abc6

Use textwrap.dedent again to keep the code clear.

77fe8dc

mgedmin reviewed Oct 8, 2013
View reviewed changes

mauritsvanrees added 2 commits October 9, 2013 10:48

Warn when we cannot parse recursive-exclude.

cc6f20a

Fix recursive-exclude when pattern does not have wildcards.

b62f61a

Fix recursive-exclude for pattern not starting with wildcard character.

f83b42d

Use the warnings module to print warnings.

7538143

Ignore warnings in one known spot in the tests. This keeps the test output clean.

mgedmin added a commit that referenced this pull request Oct 10, 2013

Merge pull request #19 from mauritsvanrees/maurits-read-manifest

58b0aaa

Read the existing MANIFEST.in file for files to ignore.

mgedmin merged commit 58b0aaa into mgedmin:master Oct 10, 2013

mauritsvanrees deleted the maurits-read-manifest branch October 10, 2013 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read the existing MANIFEST.in file for files to ignore. #19

Read the existing MANIFEST.in file for files to ignore. #19

mauritsvanrees commented Oct 4, 2013

coveralls commented Oct 4, 2013

mauritsvanrees commented Oct 4, 2013

mgedmin Oct 6, 2013

mgedmin commented Oct 6, 2013

coveralls commented Oct 7, 2013

mauritsvanrees commented Oct 7, 2013

mgedmin Oct 7, 2013

mgedmin commented Oct 7, 2013

coveralls commented Oct 8, 2013

mauritsvanrees commented Oct 8, 2013

mgedmin Oct 8, 2013

mauritsvanrees Oct 9, 2013

mauritsvanrees Oct 9, 2013

mgedmin Oct 9, 2013

coveralls commented Oct 9, 2013

mauritsvanrees commented Oct 9, 2013

mgedmin commented Oct 9, 2013

mgedmin commented Oct 9, 2013

coveralls commented Oct 10, 2013

mauritsvanrees commented Oct 10, 2013

mgedmin commented Oct 10, 2013

coveralls commented Oct 10, 2013

mauritsvanrees commented Oct 10, 2013

mgedmin commented Oct 10, 2013

mauritsvanrees commented Oct 10, 2013

myint commented Oct 11, 2013

myint commented Oct 11, 2013

Read the existing MANIFEST.in file for files to ignore. #19

Read the existing MANIFEST.in file for files to ignore. #19

Conversation

mauritsvanrees commented Oct 4, 2013

coveralls commented Oct 4, 2013

mauritsvanrees commented Oct 4, 2013

mgedmin Oct 6, 2013

Choose a reason for hiding this comment

mgedmin commented Oct 6, 2013

coveralls commented Oct 7, 2013

mauritsvanrees commented Oct 7, 2013

mgedmin Oct 7, 2013

Choose a reason for hiding this comment

mgedmin commented Oct 7, 2013

coveralls commented Oct 8, 2013

mauritsvanrees commented Oct 8, 2013

mgedmin Oct 8, 2013

Choose a reason for hiding this comment

mauritsvanrees Oct 9, 2013

Choose a reason for hiding this comment

mauritsvanrees Oct 9, 2013

Choose a reason for hiding this comment

mgedmin Oct 9, 2013

Choose a reason for hiding this comment

coveralls commented Oct 9, 2013

mauritsvanrees commented Oct 9, 2013

mgedmin commented Oct 9, 2013

mgedmin commented Oct 9, 2013

coveralls commented Oct 10, 2013

mauritsvanrees commented Oct 10, 2013

mgedmin commented Oct 10, 2013

coveralls commented Oct 10, 2013

mauritsvanrees commented Oct 10, 2013

mgedmin commented Oct 10, 2013

mauritsvanrees commented Oct 10, 2013

myint commented Oct 11, 2013

myint commented Oct 11, 2013