sdist generates bad MANIFEST on Windows

[03bde425-37ce-4291-88bd-d6cecc46a30e]

BPO	828450
Nosy	@tarekziade, @merwok, @zooba, @dstufft
Files	change_path_separator_fullversion.patch: full version manifest-sep.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = 'https://github.com/merwok' closed_at = created_at = labels = ['easy', 'type-bug', 'library'] title = 'sdist generates bad MANIFEST on Windows' updated_at = user = 'https://bugs.python.org/jhylton' ``` bugs.python.org fields: ```python activity = actor = 'steve.dower' assignee = 'eric.araujo' closed = True closed_date = closer = 'steve.dower' components = ['Distutils'] creation = creator = 'jhylton' dependencies = [] files = ['22333', '22336'] hgrepos = [] issue_num = 828450 keywords = ['patch', 'easy'] message_count = 23.0 messages = ['18734', '111045', '117680', '133779', '133829', '133840', '133841', '133842', '133920', '133985', '134076', '134078', '134128', '137974', '138151', '138176', '138194', '138204', '144083', '144380', '144381', '221819', '386305'] nosy_count = 8.0 nosy_names = ['jhylton', 'tarek', 'eric.araujo', 'thomas.holmes', 'santoso.wijaya', 'higery', 'steve.dower', 'dstufft'] pr_nums = [] priority = 'normal' resolution = 'out of date' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue828450' versions = ['Python 2.7', 'Python 3.4', 'Python 3.5'] ```

[03bde425-37ce-4291-88bd-d6cecc46a30e]

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.

[merwok]

Thanks for the report, and sorry noone replied earlier. Could you perhaps make a diff against test_sdist.py to confirm this bug?

[merwok]

Adding the “easy” keyword to encourage someone to write a test.

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

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.

[merwok]

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?

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

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.

[merwok]

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.

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

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.

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

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.

[merwok]

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).

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

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.

[merwok]

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 :)

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

> 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.

[merwok]

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?

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

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

[merwok]

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.

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

OK. I recreated a full version patch. I'll remove old patches.

[merwok]

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.

[merwok]

I’ve just found another problem with MANIFEST files created in Windows: they use CRLF.

[eb02b753-2a7a-4175-b622-4ae18d2cab69]

>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?

[merwok]

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.

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

@Éric did you ever ask on distutils-sig if MANIFEST is meant to be cross-platform?

[zooba]

Distutils is now deprecated (see PEP-632) and all tagged issues are being closed. From now until removal, only release blocking issues will be considered for distutils.

If this issue does not relate to distutils, please remove the component and reopen it. If you believe it still requires a fix, most likely the issue should be re-reported at https://github.com/pypa/setuptools