[BUG] wheel install_as_egg does not honor file mode

[delijati]

Pull Request Checklist

• Changes have tests
• [?] News fragment added in changelog.d/.
  (See documentation for details)

See #3166 for more info.

[delijati]

@abravalheri can you take a look :)

[abravalheri]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[abravalheri]

Hi @delijati thank you very much for creating the PR.
I can try to have a look, but this is one of the areas of setuptools that I am not familiar with.

[abravalheri]

Hi @delijati, I did change the PR a little bit for the following reasons:

• I split a line that was too long to prevent flake8 from failing
• I reordered the commits so I could have a look on the tests failing prior to the fix being implemented.

I hope you don't mind these changes. Please feel free to revert if that works better for you.

[abravalheri]

Thank you very much for the PR. I had a look on the code and it looks good. I just left a few notes bellow:

General remarks about the reported issue/proposed fix:

• The install_as_egg interface is a non-public interface used internally by setuptools.
  This function, as the name suggests, is very likely to be deprecated, but is currently being used by easy_install.
• easy_install despite being deprecated, is still an internal mechanism used by setuptools for editable installs.
• In this context, this PR makes sense, since it can fix potential issues for users using editable installs.

Review comments:

• The CI output seems to indicate that this implementation is OS-specific. Is this approach safe to use on Windows environments? What would be the impact for the users?
• The implementation overrides a non-public API of ZipFile.
  I understand that the method is not completely private, but this also means that we cannot rely on this approach being stable across future Python versions.
  Is this a well-known approach? Is it documented somewhere or used by other big project in the Python community?
  I wonder if instead we could use the already existing code in:

  setuptools/setuptools/archive_util.py

  Line 91 in 98728b1

  def unpack_zipfile(filename, extract_dir, progress_filter=default_filter):

[delijati]

Thank you very much for the PR. I had a look on the code and it looks good. I just left a few notes bellow:

Hi thanks for looking into that PR.

General remarks about the reported issue/proposed fix:

* The `install_as_egg` interface is a non-public interface used internally by setuptools.

I know but for now it is the only solution and already used here: buildout/buildout#604

  This function, as the name suggests, is very likely to be deprecated, but is currently being used by `easy_install`.

* `easy_install` despite being deprecated, is still an internal mechanism used by setuptools for editable installs.

* In this context, this PR makes sense, since it can fix potential issues for users using editable installs.

Review comments:

* The CI output seems to indicate that this implementation is OS-specific. Is this approach safe to use on Windows environments? What would be the impact for the users?

I would skip the test on windows (will add that) as it does "nothing" there. [3]

* The implementation overrides a non-public API of `ZipFile`.
  I understand that the method is not completely private, but this also means that we cannot rely on this approach being stable across future Python versions.
  Is this a well-known approach? Is it documented somewhere or used by other big project in the Python community?

It is an old Python Bug that seams to be fixed in >= 3.11 [1] And fixes for it are all over the place [2]

  I wonder if instead we could use the already existing code in: https://github.com/pypa/setuptools/blob/98728b103a5c5940abbc2fab60ccfc5fa265d280/setuptools/archive_util.py#L91

I looked into it but that would mean to change the api of all call used by install_as_egg. So i would rather stick with my solution.

[1] https://bugs.python.org/issue15795
[2] https://stackoverflow.com/questions/39296101/python-zipfile-removes-execute-permissions-from-binaries/54748564
[3] "Note Although Windows supports chmod(), you can only set the file’s read-only flag with it (via the stat.S_IWRITE and stat.S_IREAD constants or a corresponding integer value). All other bits are ignored."

[abravalheri]

Thank you very much @delijati for working in the PR and tests.

I think this change should be mostly fine since it is just applying the same existing behaviour that already exists in setuptools.

It is also relevant since we still depend on this code for editable installs.

[abravalheri]

Hi @delijati, would you like to add a news fragment for the CHANGELOG?

[delijati]

Hi @delijati, would you like to add a news fragment for the CHANGELOG?

Done

[abravalheri]

Thank you very much @delijati.

We have some pending changes that were already committed to the main branch lately but not released (and other changes scheduled to be merged soon), so I think it would be if we delay this change a little bit¹.

It would also give the chance for other maintainers with more experience in the codebase to review it.
Sorry for that, but I hope that works for you.

Footnotes

1. In the case something goes wrong it is best if we have less changes interacting to cause issues... ↩

[abravalheri]

Hi @befeleme, I was wondering if a change like this would impact OS packagers. Since you seem to be working with packaging at Red Hat, do you have any thoughts about that?

Here we tried to re-use some existing code that handles the unpacking of the wheels, which I honestly feel like it is the right thing to do.

However this existing code does explicitly disallow file names in wheels that start with / or ...

Do you think there might be any existing packages that are forcing file names inside the zip file to install files at arbitrary locations?

I had a look on the zip specification and it explicitly disallows names starting with / (section 4.4.17), but we never know...

[befeleme]

Hi @abravalheri,
we've discussed the issue briefly with @hroncok who has a far exceeding expertise regarding the Fedora ecosystem.
It looks like the affected code is only invoked in install_as_egg function. In our RPM build process, we don't install wheels using setuptools but with pip, so AFAIK from the downstream's packager perspective the change shall not affect us.

Do you think there might be any existing packages that are forcing file names inside the zip file to install files at arbitrary locations?

If such packages exist, we believe it'd better if they were forbidden to do that. So this feels as an improvement.

[abravalheri]

Thank you very much for looking this up @befeleme!

I am inclined to merge this PR not exactly in the next release but in the following one, unless the other maintainers/OS re-packagers have any objections.

[abravalheri]

Hi @mgorny, @FFY00 I just would like to double check with you if the changes introduced here will somehow impact negatively on Gentoo, Archlinux or any other OS/distribution you help to maintain.

This change may affect:

• uses that still rely on the legacy setup.py install
• uses that rely on setup.py develop / pip install -e . (for projects that use setuptools).

What the change will do:

• Preserve file mode when extracting a wheel file
• Error-out if the wheel contain absolute file paths or "../" (I am not really sure those can be be found in wheels due to the wheel and zip standards).

[mgorny]

Heh, I'm somewhat confused by this summary. For new packages we tend to use installer (that does respect wheel's permissions), for old packages we use plain setup.py install and I don't think that uses wheels in any way.

[abravalheri]

Thank you very much @mgorny for the insights.

Regarding the confusion: I am under the impression that setup.py install uses easy_install under the hood so there might be some shared behaviour. But yes they might not be related at all as you pointed out.

[FFY00]

I think a wheel is generated internally and then installed as a egg in setup.py install, right? Otherwise I don't see how wheel has to do with this.
Even is this is the case, the wheel would be generated by setuptools, so it has control of how it is generated.

Wasn't easy_install removed already?

[abravalheri]

Thank you very much @FFY00 for having a look. So I think it is safe to assume at this point that the change does not affect OS distributions.

Wasn't easy_install removed already?

easy_install is no longer public, but still used internally for setup.py install/develop.

[delijati]

I think a wheel is generated internally and then installed as a egg in setup.py install, right? Otherwise I don't see how wheel has to do with this. Even is this is the case, the wheel would be generated by setuptools, so it has control of how it is generated.

Wasn't easy_install removed already?

The internal install_as_egg is used here buildout/buildout#604 that's where the error occurred

[FFY00]

Ah, I see! That makes much more sense to me. I am still not sure if install_as_egg is used by setup.py install, but this patch shouldn't really have any negative impact anyway since we are not working with external wheels.