New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

write only relative path in 'RECORD' #3448

Merged
merged 4 commits into from Feb 7, 2016

Conversation

Projects
None yet
4 participants
@stonebig
Contributor

stonebig commented Feb 2, 2016

RECORD now contains only relative path.
some bugs triggered by a bleeding edge usage of python may disappear a result (#3433)

Review on Reviewable

stonebig added some commits Feb 2, 2016

write only relative path in 'RECORD'
RECORD now contains only relative path.
some bugs triggered by a bleeding edge usage of python may disappear a result (#3433)
@stonebig

This comment has been minimized.

Show comment
Hide comment
@stonebig

stonebig Feb 2, 2016

Contributor

hum, I don't think I can figure out the missing pieces for 2.6 and pypy.

Contributor

stonebig commented Feb 2, 2016

hum, I don't think I can figure out the missing pieces for 2.6 and pypy.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Feb 2, 2016

Member

I'm guessing your change meant that the uninstall couldn't find the 2 files noted in the test, and so they didn't get uninstalled. Not sure why 2.6/PyPy would be the only ones with that behaviour...

Member

pfmoore commented Feb 2, 2016

I'm guessing your change meant that the uninstall couldn't find the 2 files noted in the test, and so they didn't get uninstalled. Not sure why 2.6/PyPy would be the only ones with that behaviour...

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Feb 2, 2016

Member

Sorry - to be clearer, you wrote something different in RECORD which is where uninstall looks to work out what to remove.

Member

pfmoore commented Feb 2, 2016

Sorry - to be clearer, you wrote something different in RECORD which is where uninstall looks to work out what to remove.

@stonebig

This comment has been minimized.

Show comment
Hide comment
@stonebig

stonebig Feb 2, 2016

Contributor

trying to simplify to one line, seems to do the trick

Contributor

stonebig commented Feb 2, 2016

trying to simplify to one line, seems to do the trick

@stonebig

This comment has been minimized.

Show comment
Hide comment
@stonebig

stonebig Feb 2, 2016

Contributor

my first pypy !

Contributor

stonebig commented Feb 2, 2016

my first pypy !

stonebig pushed a commit to stonebig/winpython that referenced this pull request Feb 2, 2016

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Feb 3, 2016

Member

Note that the RECORD file (paths) should follow these semantics, as defined in PEP 376.

the file's path

  • a '/'-separated path, relative to the base location, if the file is under the base location.
  • a '/'-separated path, relative to the base location, if the file is under the installation prefix AND if the base location is a subpath of the installation prefix.
  • an absolute path, using the local platform separator
Member

Ivoz commented Feb 3, 2016

Note that the RECORD file (paths) should follow these semantics, as defined in PEP 376.

the file's path

  • a '/'-separated path, relative to the base location, if the file is under the base location.
  • a '/'-separated path, relative to the base location, if the file is under the installation prefix AND if the base location is a subpath of the installation prefix.
  • an absolute path, using the local platform separator
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Feb 3, 2016

Member

@Ivoz I'm pretty sure that's what normpath sorts out. Although it looks like it might get it wrong if it has to fall back to an absolute path (it uses / rather than the local path separator in that case) but that's not the fault of this patch.

OTOH, there doesn't seem to be any sort of check for the condition "if the file is under the installation prefix AND if the base location is a subpath of the installation prefix" in normpath, so blindly applying normpath might be wrong (both here and for the installed files...)

The installed files have always used normpath though, and the tests all succeed, so I'm inclined to say that it's probably OK. If someone wants to add extra "torture tests" checking weird combinations where the base location is not a subpath of the installation prefix, I'd be fine with that - and if that reveals bugs then we can fix them. But I'm inclined to consider that as a separate piece of work.

I'm willing to merge this unless anyone has any objections - @Ivoz are you OK with it given my comments above?

Member

pfmoore commented Feb 3, 2016

@Ivoz I'm pretty sure that's what normpath sorts out. Although it looks like it might get it wrong if it has to fall back to an absolute path (it uses / rather than the local path separator in that case) but that's not the fault of this patch.

OTOH, there doesn't seem to be any sort of check for the condition "if the file is under the installation prefix AND if the base location is a subpath of the installation prefix" in normpath, so blindly applying normpath might be wrong (both here and for the installed files...)

The installed files have always used normpath though, and the tests all succeed, so I'm inclined to say that it's probably OK. If someone wants to add extra "torture tests" checking weird combinations where the base location is not a subpath of the installation prefix, I'd be fine with that - and if that reveals bugs then we can fix them. But I'm inclined to consider that as a separate piece of work.

I'm willing to merge this unless anyone has any objections - @Ivoz are you OK with it given my comments above?

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Feb 6, 2016

Contributor

Since installed and changed files currently go through normpath, it would make sense for generated files to do the same.

Contributor

xavfernandez commented Feb 6, 2016

Since installed and changed files currently go through normpath, it would make sense for generated files to do the same.

pfmoore added a commit that referenced this pull request Feb 7, 2016

Merge pull request #3448 from stonebig/patch-1
write only relative path in 'RECORD'

@pfmoore pfmoore merged commit 61f75c4 into pypa:develop Feb 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Feb 7, 2016

Member

Thanks for the contribution @stonebig !

Member

pfmoore commented Feb 7, 2016

Thanks for the contribution @stonebig !

@stonebig

This comment has been minimized.

Show comment
Hide comment
@stonebig

stonebig Feb 7, 2016

Contributor

super !

Contributor

stonebig commented Feb 7, 2016

super !

@xavfernandez xavfernandez added this to the 8.0.3 milestone Feb 8, 2016

xavfernandez added a commit to xavfernandez/pip that referenced this pull request Feb 24, 2016

Merge pull request pypa#3448 from stonebig/patch-1
write only relative path in 'RECORD'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment