classification
Title: sdist generates bad MANIFEST on Windows
Type: behavior Stage: patch review
Components: Distutils Versions: Python 3.4, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: dstufft, eric.araujo, higery, jhylton, santoso.wijaya, tarek, thomas.holmes
Priority: normal Keywords: easy, patch

Created on 2003-10-22 19:30 by jhylton, last changed 2019-03-16 00:04 by BreamoreBoy.

Files
File name Uploaded Description Edit
change_path_separator_fullversion.patch higery, 2011-06-12 01:44 full version review
manifest-sep.diff eric.araujo, 2011-06-12 10:55
Messages (22)
msg18734 - (view) Author: Jeremy Hylton (jhylton) (Python triager) Date: 2003-10-22 19:30
The generated MANIFEST file has Windows slashes
separating path components.  If you use this MANIFEST
on Unix, it will bomb complaining the the files are not
"regular files."  If you generate a MANIFEST on Unix,
it will work on Windows.

Presumably the solution is to replace \ with / on Windows.
msg111045 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-07-21 13:03
Thanks for the report, and sorry noone replied earlier. Could you perhaps make a diff against test_sdist.py to confirm this bug?
msg117680 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-09-30 00:36
Adding the “easy” keyword to encourage someone to write a test.
msg133779 - (view) Author: higery (higery) Date: 2011-04-15 04:06
I will join GSOC2011 and I find this bug is a good test for me to submit my patch as a scoring judgment.

I have created a diff file to confirm this bug, but setuptools may have already fix it, because when using 'python setup.py sdist' to create the source distribution, file paths in SOURCES.txt are correctly with '/' as separator. However, when running the test_sdist.py tests, the problem Jeremy raised exists. 

So, it means that distutils still has this problem, but setuptools fix it.
msg133829 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-04-15 14:57
setuptools sdist uses a wholly different machinery than distutils, so it’s a red herring.

Have you tested that your patch does reproduce the bug?  From the diff header, I see that you’ve patched your installed Python instead of using a developpers’ environment.  The guide at docs.python.org/devguide should help you get set up.

If the patch does reproduce the bug, can you also fix it?
msg133840 - (view) Author: higery (higery) Date: 2011-04-15 15:33
Yes, the test fails and the output msg is: 
AssertionError: '\\' unexpectedly found in '# file GENERATED by distutils, do NOT edit\nREADME\nsetup.py\nsomecode\\__init__.py\n'

It means that distutils generates MANIFEST with '\' as file path separator.

OK, I'll try to make the diff and patch against the dev environment to fix this bug ASAP.
msg133841 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-04-15 15:34
Thanks!  I would also like it if you could use a more specific test, comparing a line with a path instead of using the overly broad assertIn, to make the intent of the test clearer.
msg133842 - (view) Author: higery (higery) Date: 2011-04-15 15:44
OK. I used this method just because I thought '\' is a special character and if it's in a file path line, then it must be the separator. 

As you say, it may be not that clear for others to know what does this test do.
msg133920 - (view) Author: higery (higery) Date: 2011-04-17 05:25
It may be just necessary to hack the write_manifest funtion in sdist.py- replace all '\' in MANIFEST with '/', thus it will not have other bad side effects, for instance, it would not change the content of self.filelist and people can also use '/' in MANIFEST template file on windows as usual.
msg133985 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-04-18 17:25
Thanks for the patch.  Follow the “review” link above to find my review.

+1 on making the smallest possible change.  The paths inside the Filelist/Manifest objects can continue to use os.sep, we only care about write_file for this bug.  Oh, and also read_file: can you add tests for MANIFEST reading?  It will be a bit more complicated: write a MANIFEST file with /-separated paths, set os.sep to \\, check that sdist works.  You should learn a lot about our tests helpers (mostly TempdirManager).
msg134076 - (view) Author: higery (higery) Date: 2011-04-19 16:49
I'm not sure it is necessary to use TempdirManager here to write tests for MANIFEST reading.
The attachment is the test diff file against my last patch with the latest version.

Detail:
Step 1: Write sample MANIFEST strings to the MANIFEST file with '/' as separator and get the filelist of this file(actually just created from the sample strings)
Step 2: Change the os.sep to '\' and then run the sdist command, and a FileList will be generated. Bad effect is MANIFEST file will be re-written: write_manifest function called(method calling route: run->get_file_list->write_manifest). Because the content in MANIFEST has already been changed, we can just use the FileList object to get the filelist, instead of construct it from reading MANIFEST file again as other tests do.
Step 3: Compare filelist_1 generated in Step1 with filelist_2 in Step2, making sure that we have replace '\' with '/' for filelist_2. Yes, we just compare the content to make sure that we has done right thing and reading MANIFEST file with '/' as separator on the platform which os.sep is '\' is ok.

That's all.
msg134078 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-04-19 16:53
> I'm not sure it is necessary to use TempdirManager here to write tests
> for MANIFEST reading.

Well, the sdist command has to run on some directory where some files exist, right?  So it’s better to do it in a temp dir that will automatically get cleaned up.

> Bad effect is MANIFEST file will be re-written:

You should read the docs for MANIFEST and sdist: a MANIFEST without the magic comment will not get overwritten (unless there’s a bug :)
msg134128 - (view) Author: higery (higery) Date: 2011-04-20 07:48
>> I'm not sure it is necessary to use TempdirManager here to write tests
>> for MANIFEST reading.

>Well, the sdist command has to run on some directory where some files >exist, right?  So it’s better to do it in a temp dir that will automatically get >cleaned up.

Because the MANIFEST file finally will be filled with file-paths which takes '/' as path separator and that's what we can make sure if we already hacked the write_manifest function in sdist.py, so we can just write the imitated paths strings  into the MANIFEST file , not needing to really create these files at first. As a test, it makes sense. So, I think we maynot need to use TempdirManager here.

>> Bad effect is MANIFEST file will be re-written:

>You should read the docs for MANIFEST and sdist: a MANIFEST without >the magic comment will not get overwritten (unless there’s a bug :)

Oh, you are right. Thus this bad effect would not exist for this test. But future discussing is still needed.
msg137974 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-09 14:41
I wanted to review the patch but it cannot be applied.  It looks like your third patch applies on top of the second.  Can you generate a full patch?
msg138151 - (view) Author: higery (higery) Date: 2011-06-11 02:57
I just recreated this patch against version 2.7, so I'm not sure it can be applied to all the listed versions.

Note: there still are two pathes, one for sdist.py and another for test_sdist.py
msg138176 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-11 18:04
> I just recreated this patch against version 2.7, so I'm not sure it
> can be applied to all the listed versions.
It’s okay, distutils in 2.7 and 3.x is very similar, I can port.

> Note: there still are two pathes, one for sdist.py and another for
> test_sdist.py
Why?  hg diff can create one patch for many files.  It’s a bit easier if you upload just one patch.  Anyway, it’s not an issue, just upload the missing patch for sdist.py.
msg138194 - (view) Author: higery (higery) Date: 2011-06-12 01:44
OK. I recreated a full version patch. I'll remove old patches.
msg138204 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-12 10:55
I have changed some things in your patch.  There are still two issues:

1) setting os.sep to \ in the tests is not enough to trigger the bug.  This means that the tests really test something only on Windows.  I’ll edit them to mock the OS layer and return paths with \, so that we can check that manifest or sdist does the conversion.

2) I am not sure we can actually change this in distutils, because of the compatibility policy.  The documentation does not say that the MANIFEST file should be portable, so I’ll ask on distutils-sig for feedback.
msg144083 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-09-15 16:03
I’ve just found another problem with MANIFEST files created in Windows: they use CRLF.
msg144380 - (view) Author: higery (higery) Date: 2011-09-21 14:39
>>I’ve just found another problem with MANIFEST files created in Windows: they use CRLF.

One cann't let Python generate MANIFEST files taking Unix-style LF as newline endings On Windows, I think.

So, does it mean even though we have already made much effort for this bug, it still cann't make MANIFEST file on different platforms cross-platform?
msg144381 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-09-21 14:42
> One cann't let Python generate MANIFEST files taking Unix-style LF as newline
> endings On Windows, I think.
Why?  Python can open it fine, and it’s not meant for human edition, so the stupidity of notepad.exe is not a problem <wink>.

> So, does it mean even though we have already made much effort for this bug, it
> still cann't make MANIFEST file on different platforms cross-platform?
The problem is that the docs never say that MANIFEST is cross-platform, so I’m not sure changing this qualify for a bug fix.  I have to find time to ask distutils-sig.

See also my point 1) in my previous message: the tests don’t test anything on non-Windows.

For d2 however we can change the MANIFEST format as we like.
msg221819 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-28 22:29
@Éric did you ever ask on distutils-sig if MANIFEST is meant to be cross-platform?
History
Date User Action Args
2019-03-16 00:04:08BreamoreBoysetnosy: - BreamoreBoy
2014-06-28 22:29:50BreamoreBoysetversions: + Python 3.4, Python 3.5, - 3rd party, Python 3.2, Python 3.3
nosy: + dstufft, BreamoreBoy

messages: + msg221819

components: - Windows, Distutils2
2011-09-21 14:42:55eric.araujosetmessages: + msg144381
2011-09-21 14:39:15higerysetmessages: + msg144380
2011-09-15 16:03:53eric.araujosetmessages: + msg144083
2011-06-30 17:08:24thomas.holmessetnosy: + thomas.holmes
2011-06-12 10:55:35eric.araujosetfiles: + manifest-sep.diff
resolution: accepted ->
messages: + msg138204
2011-06-12 01:46:23higerysetfiles: - test_manifest_reading_sdist.diff
2011-06-12 01:46:15higerysetfiles: - change_path_separator_in_MANIFEST.patch
2011-06-12 01:44:35higerysetfiles: + change_path_separator_fullversion.patch

messages: + msg138194
2011-06-11 18:04:19eric.araujosetmessages: + msg138176
2011-06-11 02:57:48higerysetfiles: + test_manifest_reading_sdist.diff

messages: + msg138151
2011-06-11 02:43:15higerysetfiles: - test_manifest_reading_sdist_v2.diff
2011-06-11 02:43:07higerysetfiles: - test_manifest_reading_sdist.diff
2011-06-09 14:41:33eric.araujosetstatus: pending -> open

messages: + msg137974
versions: - Python 3.1
2011-04-29 16:00:44eric.araujosetstatus: open -> pending
resolution: accepted
stage: test needed -> patch review
2011-04-24 14:54:21higerysetfiles: + test_manifest_reading_sdist_v2.diff
2011-04-20 07:48:29higerysetmessages: + msg134128
2011-04-19 16:53:43eric.araujosetmessages: + msg134078
2011-04-19 16:49:18higerysetfiles: + test_manifest_reading_sdist.diff

messages: + msg134076
2011-04-19 08:30:43higerysetfiles: + change_path_separator_in_MANIFEST.patch
2011-04-19 08:30:17higerysetfiles: - change_path_separator_in_MANIFEST.patch
2011-04-18 17:25:22eric.araujosetmessages: + msg133985
2011-04-17 15:35:46higerysetfiles: + change_path_separator_in_MANIFEST.patch
2011-04-17 15:34:38higerysetfiles: - change_path_separator_in_MANIFEST.patch
2011-04-17 05:25:19higerysetfiles: + change_path_separator_in_MANIFEST.patch

messages: + msg133920
2011-04-17 05:19:12higerysetfiles: - test_sdist.diff
2011-04-15 17:30:58santoso.wijayasetnosy: + santoso.wijaya
2011-04-15 15:44:39higerysetmessages: + msg133842
2011-04-15 15:34:58eric.araujosetassignee: tarek -> eric.araujo
messages: + msg133841
2011-04-15 15:33:29higerysetmessages: + msg133840
2011-04-15 14:57:10eric.araujosetmessages: + msg133829
versions: + Python 3.3
2011-04-15 04:06:04higerysetfiles: + test_sdist.diff

nosy: + higery
messages: + msg133779

keywords: + patch
2010-09-30 00:36:01eric.araujosetkeywords: + easy

messages: + msg117680
versions: + 3rd party
2010-08-18 22:57:01BreamoreBoysettype: behavior
versions: + Python 3.1, Python 2.7, Python 3.2, - Python 2.6, Python 2.5
2010-07-21 13:03:29eric.araujosetnosy: + eric.araujo
messages: + msg111045

components: + Distutils2
stage: test needed
2009-02-11 02:52:32ajaksu2setassignee: tarek
nosy: + tarek
2008-01-05 18:02:19christian.heimessetcomponents: + Windows
versions: + Python 2.6, Python 2.5
2003-10-22 19:30:46jhyltoncreate