classification
Title: Can't add files with spaces under bdist_rpm
Type: behavior Stage: needs patch
Components: Distutils Versions: Python 3.5, Python 3.4, Python 3.3, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: a.badger, antonio, asandvig, eric.araujo, jason.coombs, johnkeyes, matheus.v.portela, r.david.murray, tarek, xiscu
Priority: normal Keywords: easy, patch

Created on 2003-09-19 09:10 by antonio, last changed 2014-04-05 12:52 by jason.coombs.

Files
File name Uploaded Description Edit
issue809163.diff johnkeyes, 2010-11-20 01:25 Unit test for including files with spaces review
issue809163v2.diff johnkeyes, 2010-11-20 23:19 Now uses TempdirManager review
issue809163v3.diff johnkeyes, 2010-11-21 09:44 Changed assertTrue to assertEquals review
test_bdist_rpm_filename_with_whitespaces.diff xiscu, 2011-01-07 21:02 Unit test for bdist_rpm that triggers the issue
updated_test_bdist_rpm_filename_with_whitespaces.diff matheus.v.portela, 2014-03-07 21:22 review
quoted_files.diff matheus.v.portela, 2014-03-09 05:36 Updated tests and work-around for double-quoting file paths review
issue809163-no-record.patch jason.coombs, 2014-04-05 12:52
Messages (22)
msg60388 - (view) Author: Antonio Beamud Montero (antonio) Date: 2003-09-19 09:10
If i try to add data files with blank spaces in its
names using a wildcard in the MANIFEST.in file, it
doesn't do the work well and add only the fisrt part of
the name file. For example:
----- MANIFEST.in ----
images/*
-----------------------
The MANIFEST file generated is ok, but when rpm util
try to process it fail... The solution is to scape the
names or quote the names that are generated inside
INSTALLED_FILES

INSTALLED_FILES
running install
running build
running install_data
writing list of installed files to 'INSTALLED_FILES'
+ /usr/lib/rpm/brp-compress
+ /usr/lib/rpm/brp-strip
+ /usr/lib/rpm/brp-strip-comment-note
Processing files: garra-configurator-0.9.12-1
error: Two files on one line:
/usr/local/apache/htdocs/images/default
error: File must begin with "/": logo-tv-1.png

msg111043 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-07-21 13:00
Thanks for the report Antonio. I’m sorry noone replied before; bugs are triaged by volunteers and distutils had no dedicated maintainer in the previous years. This needs a test to make sure the bug still exists, and if confirmed a patch to fix it. I don’t have time right now but it will be done. Thanks again.
msg117685 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-09-30 01:23
Adding the “easy” keyword so that a beginner or sprinter can find this and write a test.  Hint: Forget the part about RPM, write a test about inclusion of file with spaces in their name.
msg121587 - (view) Author: John Keyes (johnkeyes) Date: 2010-11-20 01:25
This is my first contribution as part of the Bug Weekend (and possibly my first to Python).

I tested this by writing a MANIFEST.in and a very small setup.py but after looking at distutils I narrowed the area down to the FileList.

I wrote a unittest and have attached the diff. I tested this on py3k.
msg121591 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-20 03:12
Thanks for diagnosis and the test patch, and welcome to the bug weekend.

Some comments:

test.support has a symbol, TESTFN, which is guaranteed to be unique for the test run and located in an appropriate writeable location.  Many tests use it to create a directory via os.mkdir and then create working files in it.  The test should also clean up the directory afterward, which can be done easily using test.support.rmtree.  Using these two test support items should simplify your test.  The other advantage of using TESTFN rather than the tempfile module is that TESTFN locates the file/directory in a place that the buildbot runner cleans up after each test run, so no cruft is left on the buildbot system even if the test runs ends abnormally in the middle of the test.
msg121626 - (view) Author: John Keyes (johnkeyes) Date: 2010-11-20 12:50
I'll change the test to use TESTFN later today. Thanks for the feedback.
msg121628 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-20 13:00
Thanks for the patch John.  Instead of low-level use of TESTFN, you can use distutils.tests.support.TempdirManager to create a temporary directory and write files to it in a few lines.
msg121785 - (view) Author: John Keyes (johnkeyes) Date: 2010-11-20 21:33
I see that distutils.tests.support.TempdirManager uses tempfile for directory creation.

What's the best course of action here? 

1. Use TESTFN or
2. Use TempdirManager or
3. Change the test to use TempdirManager and change TempdirManager to use TESTFN instead of tempfile.

Thanks.
msg121790 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-20 22:01
TempdirManager hasn’t caused buildbots failures so far, so there is no need to change it.  You can use it, it’s convenient and works :)
msg121811 - (view) Author: John Keyes (johnkeyes) Date: 2010-11-20 23:19
This patch uses TempdirManager to handle the temporary directory and file creation.
msg121816 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-20 23:56
TempdirManager is new, which is why I forgot about it.  It works with the buildbots because it is a context manager, so if things go badly the cleanup code is still going to run and delete the cruft.  Since the distutils tests are unlikely to hang (which is the only time the cleanup code would not get run), it should be perfectly fine.
msg121857 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-21 03:56
Looks good.  You’ll want to use assertEqual here:
    self.assertTrue(1, len(file_list.files))

Do you want to patch filelist.py now?
msg121892 - (view) Author: John Keyes (johnkeyes) Date: 2010-11-21 09:44
Gah! Silly mistake with the assert.

What filelist.py patch are you referring to? I've tested this on py3k with no changes to filelist.py.

Thanks for your patience.
msg122021 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-11-21 23:34
assertEquals is deprecated, I’ll change to assertEqual.

> What filelist.py patch are you referring to? I've tested this on py3k
> with no changes to filelist.py.
The goal of a test is to show an error so that it can be fixed.  In this case, the test is still useful to make sure things work, especially in distutils2 where we are changing the filelist (now manifest) module a lot.

I think now that my comment “forget RPM” was wrong.  This is not a problem with RPM, but with bdist_rpm, so even if filelist behaves correctly with spaces, bdist_rpm does not.  If there is a way to make rpm read one line at a time, we should use it.

Toshio: Do you have an idea how to do that?  Otherwise we’ll close as wontfix and forward to the bdist_rpm2 project, where they have more freedom to change code.
msg125709 - (view) Author: xiscu (xiscu) Date: 2011-01-07 21:02
This is also my first contribution.

The attached file is a diff against :
- http://code.python.org/hg/branches/py3k/

The actual tip was:
- 9489:4db13b4e76aa

The patch adds only a test that triggers the issue.

Please feel free to comment on that commit (so I can improve)

Thanks
msg212811 - (view) Author: Matheus Vieira Portela (matheus.v.portela) * Date: 2014-03-06 15:38
Is this issue still going on?
msg212869 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-03-07 10:29
Yes, this issue is not addressed.  A test is added by the latest patch and reproduces the issue; now bdist_rpm should be changed to make the test pass.  See also my previous comment.
msg212900 - (view) Author: Matheus Vieira Portela (matheus.v.portela) * Date: 2014-03-07 21:22
I tried to apply the last patch but it returned me and error of failing hunk. I think it was based on an old version of the test_bdist_rpm.py file.

Hence, I made this updated version of the patch and could get the expected failure during the tests.

I should try and solve this bug soon.
msg212956 - (view) Author: Matheus Vieira Portela (matheus.v.portela) * Date: 2014-03-09 05:36
As far as I noticed, in bdist_rpm.py, the _make_spec_file() method generates an .spec file for RPM build and attaches the install script to "setup.py install -O1 --root=$RPM_BUILD_ROOT --record=INSTALLED_FILES".

Later, the .spec refers to the record file as the one which contains all necessary files to build the RPM package with the directive "%files -f INSTALLED_FILES". If any path contains an space character, it will triggers an error.

I could fix this problem by attaching to the installation a small sed script that appends double quotes to the beginning and the end of each line of INSTALLED_FILES. I'm not sure whether this is allowed though. Any ideas how to make this more Pythonic without using sed?

Apparently, it works since the test passed. However, there is an warning which I have no idea what it means: "unknown, 0: Warning: using regular magic file `/etc/magic'".

I attached the diff file with the changes so far.
msg213206 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-03-12 06:43
Previous comment: “If there is a way to make rpm read one line at a time, we should use it.”

If there isn’t such an option, and if sed is guaranteed to be installed on rpm-based systems, we can try the sed script.
msg215597 - (view) Author: Jason R. Coombs (jason.coombs) * (Python committer) Date: 2014-04-05 12:22
In issue21153, I explored this problem as well, before being aware of this issue. I had searched for bdist_rpm.

I've since confirmed that the sed approach is viable. For example, the following works around the issue by overriding the install_script with the sed technique:

$ cat > install.txt
python setup.py install -O1 --root=$RPM_BUILD_ROOT --record=INSTALLED_FILES
sed -i -e 's/.*/"\0"/' INSTALLED_FILES

$ python setup.py bdist_rpm --install-script install.txt

I agree that sed can probably be assumed to be present, though it would be nicer not to depend on it.

It may also be possible not to have to list the files explicitly. In reading the rpm docs (http://www.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html), I discovered that rpm will traverse a directory, adding it and its contents. Perhaps the whole routine of recording installed files and then listing them explicitly can be replaced by simply referencing the build root.
msg215601 - (view) Author: Jason R. Coombs (jason.coombs) * (Python committer) Date: 2014-04-05 12:52
I decided to test my proposition. It seems that simply specifying / for %files leads to the RPM including parent directories. Here's the output I get when running my test with the sed workaround:

$ rpm -q -filesbypkg -p dist/issue21153-package-0.0.0-1.noarch.rpm
issue21153-package        /usr/local/lib/python2.7/dist-packages/issue21153_package-0.0.0.egg-info
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/__init__.py
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/__init__.pyc
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/__init__.pyo
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/file with spaces.py
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/file with spaces.pyc
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/file with spaces.pyo

If instead I apply the no-record patch (attached), I can run bdist_rpm (not using sed workaround) without errors, but the resulting RPM has more directories specified:

$ rpm -q -filesbypkg -p dist/issue21153-package-0.0.0-1.noarch.rpm
issue21153-package        /
issue21153-package        /usr
issue21153-package        /usr/local
issue21153-package        /usr/local/lib
issue21153-package        /usr/local/lib/python2.7
issue21153-package        /usr/local/lib/python2.7/dist-packages
issue21153-package        /usr/local/lib/python2.7/dist-packages/issue21153_package-0.0.0.egg-info
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/__init__.py
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/__init__.pyc
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/__init__.pyo
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/file with spaces.py
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/file with spaces.pyc
issue21153-package        /usr/local/lib/python2.7/dist-packages/pkg/file with spaces.pyo

Perhaps that's acceptable. If it is, it makes the code simpler and cleaner.

Can someone comment on the impact of the presence of those parent directories in the RPM?
History
Date User Action Args
2014-04-05 12:52:14jason.coombssetfiles: + issue809163-no-record.patch

messages: + msg215601
2014-04-05 12:22:16jason.coombssetnosy: + jason.coombs

messages: + msg215597
title: Can't add files with spaces -> Can't add files with spaces under bdist_rpm
2014-03-12 06:43:08eric.araujosetmessages: + msg213206
2014-03-09 05:36:26matheus.v.portelasetfiles: + quoted_files.diff

messages: + msg212956
2014-03-07 21:22:33matheus.v.portelasetfiles: + updated_test_bdist_rpm_filename_with_whitespaces.diff

messages: + msg212900
2014-03-07 10:29:00eric.araujosetstage: patch review -> needs patch
messages: + msg212869
components: - Distutils2
versions: + Python 3.3, Python 3.4, Python 3.5, - 3rd party, Python 3.1, Python 3.2
2014-03-06 15:38:41matheus.v.portelasetnosy: + matheus.v.portela
messages: + msg212811
2011-01-07 21:02:28xiscusetfiles: + test_bdist_rpm_filename_with_whitespaces.diff
nosy: + xiscu
messages: + msg125709

2010-11-21 23:34:57eric.araujosetnosy: + a.badger
messages: + msg122021
2010-11-21 09:44:48johnkeyessetfiles: + issue809163v3.diff

messages: + msg121892
2010-11-21 03:56:03eric.araujosetmessages: + msg121857
2010-11-20 23:56:34r.david.murraysetmessages: + msg121816
2010-11-20 23:19:28johnkeyessetfiles: + issue809163v2.diff

messages: + msg121811
2010-11-20 22:01:14eric.araujosetmessages: + msg121790
2010-11-20 21:33:47johnkeyessetmessages: + msg121785
2010-11-20 13:00:17eric.araujosetassignee: tarek -> eric.araujo
messages: + msg121628
stage: needs patch -> patch review
2010-11-20 12:50:27johnkeyessetmessages: + msg121626
2010-11-20 03:12:03r.david.murraysetnosy: + r.david.murray

messages: + msg121591
stage: test needed -> needs patch
2010-11-20 01:25:53johnkeyessetfiles: + issue809163.diff

nosy: + johnkeyes
messages: + msg121587

keywords: + patch
2010-10-07 09:51:00asandvigsetnosy: + asandvig
2010-09-30 01:23:23eric.araujosetkeywords: + easy

messages: + msg117685
versions: + 3rd party, Python 3.1, Python 2.7, Python 3.2
2010-07-21 13:00:24eric.araujosetassignee: tarek
components: + Distutils2
versions: - Python 2.6, Python 3.0, Python 3.1, Python 2.7
nosy: + eric.araujo, tarek

messages: + msg111043
stage: test needed
2009-02-04 16:45:40akitadasettype: behavior
versions: + Python 2.6, Python 3.0, Python 3.1, Python 2.7, - Python 2.3
2003-09-19 09:10:19antoniocreate