Record installed files in a deterministic order #4667

bochecha · 2017-08-11T11:36:17Z

Installed files are recorded by Pip in the order the underlying tool
(Distutils, Setuptools, ...) recorded them.

Unfortunately, at least Setuptools doesn't record them in a
deterministic order in the case of a directory being installed, as it
uses os.walk to find the list of files.

We could fix all those underlying tools to record their files in a
deterministic order in all situations. But fixing it once here in Pip
for all tools is certainly simpler and more future-proof.

This makes the installation more reproducible, and therefore more
verifiable.

Note: I did not add a NEWS file fragment because I'm not sure this deserves being mentioned. If you think it does, I'll be happy to add one. 😃

pradyunsg · 2017-08-12T07:27:45Z

I'm not sure this deserves being mentioned.

While I don't know if this functionality is something we want to add to pip (I'm not the right person to comment on that), it does seem to me that adding a news fragment would be needed.

bochecha · 2017-08-20T13:13:33Z

@pradyunsg Thanks, I've added a news fragment then.

I picked the bugfix type because unreproducible builds/installs feel like a bug to me. I'm happy to change it to whatever people here think would be more appropriate. (maybe feature or trivial?)

pradyunsg · 2017-08-20T13:38:31Z

maybe feature

I'd say so but I won't push on that -- I'd wait for a core committer to comment on this. :)

BrownTruck · 2017-09-01T17:00:02Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

xavfernandez · 2017-10-17T20:53:49Z

This would also need a test to make sure this does not get dropped at the next refactor.

bochecha · 2017-10-23T10:39:06Z

This would also need a test to make sure this does not get dropped at the next refactor.

I'd be happy to add a test for this, but I'm not sure where would be the most appropriate. tests/functional/test_install.py maybe?

pradyunsg · 2017-10-23T10:44:46Z

Yep! It'd be nice if you add a doc-string to it. :)

bochecha · 2017-10-24T09:52:58Z

Test added in the latest push to this branch. Let me know if there's anything else I can do to get this merged. 🙂

pradyunsg

LGTM. :)

pradyunsg · 2017-10-24T09:56:20Z

news/4667.bugfix

@@ -0,0 +1,2 @@
+Made the order of recorded installed files deterministic, to make installations


I'd phrase it as:

pip now records installed files in a deterministic manner improving reproducibility.

Done in the latest push to this branch.

pradyunsg · 2017-10-24T10:06:02Z

I'll wait for another review from @pypa/pip-committers before merging. :)

pfmoore · 2017-10-24T10:15:10Z

src/pip/_internal/req/req_install.py

@@ -802,6 +802,7 @@ def prepend_root(path):
                    new_lines.append(
                        os.path.relpath(prepend_root(filename), egg_info_dir)
                    )
+            new_lines = sorted(new_lines)


Better to make this an actual list (new_lines.sort()) rather than a generator? I guess the generator works at the moment, but it may cause confusion for future maintenance that new_lines isn't an actual list after this point.

sorted returns a list https://docs.python.org/3.6/library/functions.html#sorted but indeed calling new_lines.sort() should be enough since it is already a list.

Ouch. Sorry, I was getting confused between sorted and reversed... Thanks for the correction.

Sure, doesn't make a difference to me... Latest version of this branch uses new_lines.sort().

xavfernandez

Renaming the different installed_files variables in the test would improve its readability :o

xavfernandez · 2017-10-24T10:30:00Z

tests/functional/test_install.py

+    result = script.pip('install', to_install, expect_error=False)
+    fspkg_folder = script.site_packages / 'fspkg'
+    egg_info = 'FSPkg-0.1.dev0-py%s.egg-info' % pyversion
+    installed_files = script.site_packages / egg_info / 'installed-files.txt'


nit: installed_files_path

xavfernandez · 2017-10-24T10:30:03Z

tests/functional/test_install.py

+    assert installed_files in result.files_created, str(result)
+
+    installed_files = result.files_created[installed_files].full
+    installed_files = [p for p in installed_files.read_text().split('\n') if p]


nit: installed_files_lines

pradyunsg · 2017-10-24T11:30:51Z

Retriggered CI and status check.

pradyunsg · 2017-10-24T15:29:37Z

Appveyor does not approve.

pfmoore · 2017-10-24T15:56:26Z

That looks like a genuine issue. How can Travis be OK with it, but Appveyor not? It doesn't seem to be a Windows-specific issue. Is that test run on both Windows and Unix?

Weird...

xavfernandez

It looks like installed_files_path is a pathlib.Path object in Unix but a simple str path in Windows ?

pfmoore · 2017-10-24T21:42:25Z

I'm guessing the problem comes from tests/lib/__init__.py in TestPipResult:

    if sys.platform == 'win32':

        @property
        def stdout(self):
            return self._impl.stdout.replace('\r\n', '\n')

        @property
        def stderr(self):
            return self._impl.stderr.replace('\r\n', '\n')

If _impl.std{out,err} is an object with a __str__ method, this will give a string on Windows, while leaving the rich object exposed on Unix :-( As far as I can tell, _impl is a scripttest.TestFileEnvironment object, so that's likely what's gone on here. I haven't debugged further into scripttest.

pfmoore · 2017-10-24T21:43:50Z

In case it's not clear, this is a bug in the pip test harness on Windows, rather than a bug in this PR. Either way, it'll need fixing, though.

bochecha · 2017-10-26T07:00:36Z

I don't have access to a Windows machine, which will make it a bit hard to fix this blindly.

So I guess I'll have to wait for someone else to fix it, before we can merge this?

pradyunsg · 2017-10-26T07:36:45Z

So I guess I'll have to wait for someone else to fix it, before we can merge this?

Yeah... I'll look into this as well over the weekend.

benoit-pierre · 2017-10-26T13:05:40Z

This is the problem:

>>> import posixpath
>>> import ntpath
>>> from tests.lib.path import Path
>>> base_path = Path(r'T:\pytest-0\test_installed_files_recorded_0\workspace')
>>> path = r'venv\Lib\site-packages\FSPkg-0.1.dev0-py3.6.egg-info\installed-files.txt'
>>> posixpath.join(Path(base_path.replace('\\', '/')), path.replace('\\', '/'))
Path('T:/pytest-0/test_installed_files_recorded_0/workspace/venv/Lib/site-packages/FSPkg-0.1.dev0-py3.6.egg-info/installed-files.txt')
>>> ntpath.join(base_path, path)
'T:\\pytest-0\\test_installed_files_recorded_0\\workspace\\venv\\Lib\\site-packages\\FSPkg-0.1.dev0-py3.6.egg-info\\installed-files.txt'

So the result of os.path.join(Path(...), str(...)) is not a Path object on Windows...

@bochecha: can you change the test code to:

    installed_files_path = result.files_created[installed_files_path].full
    installed_files_lines = [
        p for p in Path(installed_files_path).read_text().split('\n') if p
    ]

?

pfmoore · 2017-10-26T13:25:07Z

@benoit-pierre Good catch. I'm actually more surprised that os.path.join does return a Path object on POSIX. I'd assume that's an accident - AIUI, Path objects are subclasses of str so I guess if posixpath.join uses the passed in path unchanged, but ntpath copies it (likely to do something like backslash replacement) this is the result.

Your suggested fix looks correct to me.

Installed files are recorded by Pip in the order the underlying tool (Distutils, Setuptools, ...) recorded them. Unfortunately, at least Setuptools doesn't record them in a deterministic order in the case of a directory being installed, as it uses os.walk to find the list of files. We could fix all those underlying tools to record their files in a deterministic order in all situations. But fixing it once here in Pip for all tools is certainly simpler and more future-proof. This makes the installation more reproducible, and therefore more verifiable.

bochecha · 2017-10-26T13:55:22Z

Latest push should fix the tests on Windows, as suggested by @benoit-pierre. Thanks!

pradyunsg · 2017-10-26T14:28:03Z

Thanks for looking into this @benoit-pierre! ^>^

xavfernandez · 2017-10-26T20:29:36Z

@bochecha thanks for the PR :)

lock · 2019-06-02T16:07:10Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg added type: enhancement Improvements to functionality state: needs discussion This needs some more discussion labels Aug 12, 2017

BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 2, 2017

pradyunsg reviewed Oct 24, 2017

View reviewed changes

pradyunsg approved these changes Oct 24, 2017

View reviewed changes

pfmoore requested changes Oct 24, 2017

View reviewed changes

xavfernandez approved these changes Oct 24, 2017

View reviewed changes

pfmoore approved these changes Oct 24, 2017

View reviewed changes

pradyunsg closed this Oct 24, 2017

pradyunsg reopened this Oct 24, 2017

xavfernandez reviewed Oct 24, 2017

View reviewed changes

xavfernandez merged commit 62875be into pypa:master Oct 26, 2017

bochecha deleted the deterministic-installed-files branch October 27, 2017 06:54

FRidh mentioned this pull request Nov 23, 2017

Order of files in the generated ZIP file is indeterministic pypa/wheel#183

Closed

bmwiedemann mentioned this pull request Jun 21, 2018

Sort list of RECORD entries #5525

Merged

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

		@@ -0,0 +1,2 @@
		Made the order of recorded installed files deterministic, to make installations

Record installed files in a deterministic order #4667

Record installed files in a deterministic order #4667

Conversation

bochecha commented Aug 11, 2017

pradyunsg commented Aug 12, 2017

bochecha commented Aug 20, 2017

pradyunsg commented Aug 20, 2017

BrownTruck commented Sep 1, 2017

xavfernandez commented Oct 17, 2017

bochecha commented Oct 23, 2017

pradyunsg commented Oct 23, 2017

bochecha commented Oct 24, 2017

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Oct 24, 2017

Choose a reason for hiding this comment

xavfernandez Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavfernandez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg commented Oct 24, 2017

pradyunsg commented Oct 24, 2017

pfmoore commented Oct 24, 2017

xavfernandez left a comment

Choose a reason for hiding this comment

pfmoore commented Oct 24, 2017

pfmoore commented Oct 24, 2017

bochecha commented Oct 26, 2017

pradyunsg commented Oct 26, 2017

benoit-pierre commented Oct 26, 2017

pfmoore commented Oct 26, 2017 • edited Loading

bochecha commented Oct 26, 2017

pradyunsg commented Oct 26, 2017

xavfernandez commented Oct 26, 2017

lock bot commented Jun 2, 2019

xavfernandez Oct 24, 2017 •

edited

Loading

pfmoore commented Oct 26, 2017 •

edited

Loading