Skip to content
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

Ignore existing script files #170

Closed
wants to merge 1 commit into from
Closed

Ignore existing script files #170

wants to merge 1 commit into from

Conversation

flode
Copy link

@flode flode commented Mar 10, 2023

When a wheel has an entry_points.txt script and another script with the same name. Then installer fails to install the package and complains about an already existing file.

Repro:
python3 -m installer --destdir tmp conjur_client-0.1.1-py3-none-any.whl

That wheel has a file conjur_client-0.1.1.data/scripts/conjur-cli that gets copied as well as the autogenerated script from entry_points.txt

[console_scripts]
conjur-cli = conjur:Cli.launch

Pip seems to not mind that and just ignore the script file. This commit does the same.

When a wheel has an entry_points.txt script and another script with the
same name. Then installer fails to install the package and complains
about an already existing file.

Repro:
python3 -m installer --destdir tmp conjur_client-0.1.1-py3-none-any.whl

Pip seems to not mind that and just ignore the script file. This commit
does the same.
@wuch-g2v
Copy link

wuch-g2v commented May 3, 2023

I have the same issue #182

@eli-schwartz
Copy link
Contributor

This is, technically, an invalid wheel. At the very least I'd expect a warning.

I don't think that bug for bug compatibility with pip is necessarily an objective, although probably pip should be saying something also...

@wuch-g2v
Copy link

wuch-g2v commented May 3, 2023

This is, technically, an invalid wheel. At the very least I'd expect a warning.

You mean that it is issue with wheel module? 🤔

@pradyunsg
Copy link
Member

Hi there! Thanks for filing this but this isn't a change we're going to be making in this project.

The corresponding behaviour is considered a bug in pip (pypa/pip#4625) and mimicking it is not intended in this library.

If you want to, you can write your own write_to_fs and mimick pip bug-for-bug.

https://github.com/python-poetry/poetry/blob/3804a131925854644731923d5c37946b215a71ec/src/poetry/installation/wheel_installer.py#L28 does this for example.

You mean that it is issue with wheel module?

No, rather that conjur_client-0.1.1-py3-none-any.whl is a wheel that has inappropriate contents, since it has two files that are somehow supposed to end up in the same location. This obviously should not work, but pip permits this as noted above.

@wuch-g2v
Copy link

wuch-g2v commented May 3, 2023

You mean that it is issue with wheel module?

No, rather that conjur_client-0.1.1-py3-none-any.whl is a wheel that has inappropriate contents, since it has two files that are somehow supposed to end up in the same location. This obviously should not work, but pip permits this as noted above.

I'm not 100% sure do I understand correctly above .. If that is true IMO it means that wheel module has the issue because it allows form .whl archive which such clash and it should detect that such situations and end with error.
Am I right?

@wuch-g2v
Copy link

wuch-g2v commented May 3, 2023

BTW just tested this PR and allowed me to install pecan but after that on testing with all my +1.k rpm packages in which packaging procedure I'm using installer on unpacking .whl to destir I found that it caused that pdm test suite started failing 😞

=================================== FAILURES ===================================
_________________ test_install_wheel_with_data_scripts[False] __________________

project = <Project '/tmp/pytest-of-tkloczko/pytest-123/test_install_wheel_with_data_s0'>
use_install_cache = False

    @pytest.mark.parametrize("use_install_cache", [False, True])
    def test_install_wheel_with_data_scripts(project, use_install_cache):
        req = parse_requirement("jmespath")
        candidate = Candidate(
            req,
            link=Link("http://fixtures.test/artifacts/jmespath-0.10.0-py2.py3-none-any.whl"),
        )
        installer = InstallManager(project.environment, use_install_cache=use_install_cache)
>       installer.install(candidate)

tests/test_installer.py:160:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../BUILDROOT/python-pdm-2.5.3-2.fc39.x86_64/usr/lib/python3.8/site-packages/pdm/installers/manager.py:33: in install
    installer(str(prepared.build()), self.environment, prepared.direct_url())
../../BUILDROOT/python-pdm-2.5.3-2.fc39.x86_64/usr/lib/python3.8/site-packages/pdm/installers/installers.py:172: in install_wheel
    _install_wheel(wheel=wheel, destination=destination, additional_metadata=additional_metadata)
../../BUILDROOT/python-pdm-2.5.3-2.fc39.x86_64/usr/lib/python3.8/site-packages/pdm/installers/installers.py:268: in _install_wheel
    install(source, destination, additional_metadata=additional_metadata or {})
/usr/lib/python3.8/site-packages/installer/_core.py:109: in install
    record = destination.write_file(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pdm.installers.installers.InstallDestination object at 0x7f29831fab20>
scheme = 'scripts', path = 'jp.py', stream = <zipfile.ZipExtFile [closed]>
is_executable = True

    def write_file(
        self,
        scheme: Scheme,
        path: Union[str, "os.PathLike[str]"],
        stream: BinaryIO,
        is_executable: bool,
    ) -> RecordEntry:
        """Write a file to correct ``path`` within the ``scheme``.

        :param scheme: scheme to write the file in (like "purelib", "platlib" etc).
        :param path: path within that scheme
        :param stream: contents of the file
        :param is_executable: whether the file should be made executable

        - Changes the shebang for files in the "scripts" scheme.
        - Uses :py:meth:`SchemeDictionaryDestination.write_to_fs` for the
          filesystem interaction.
        """
        path_ = os.fspath(path)

        if scheme == "scripts":
            with fix_shebang(stream, self.interpreter) as stream_with_different_shebang:
>               return self.write_to_fs(
                    scheme, path_, stream_with_different_shebang, is_executable, True
                )
E               TypeError: write_to_fs() takes 5 positional arguments but 6 were given

/usr/lib/python3.8/site-packages/installer/destinations.py:204: TypeError
---------------------------- Captured stdout setup -----------------------------
Changes are written to pyproject.toml.
Changes are written to pyproject.toml.
------------------------------ Captured log call -------------------------------
INFO     unearth.preparer:preparer.py:309 Downloading <Link http://fixtures.test/artifacts/jmespath-0.10.0-py2.py3-none-any.whl (from None)> (size unknown)
__________________ test_install_wheel_with_data_scripts[True] __________________

project = <Project '/tmp/pytest-of-tkloczko/pytest-123/test_install_wheel_with_data_s1'>
use_install_cache = True

    @pytest.mark.parametrize("use_install_cache", [False, True])
    def test_install_wheel_with_data_scripts(project, use_install_cache):
        req = parse_requirement("jmespath")
        candidate = Candidate(
            req,
            link=Link("http://fixtures.test/artifacts/jmespath-0.10.0-py2.py3-none-any.whl"),
        )
        installer = InstallManager(project.environment, use_install_cache=use_install_cache)
>       installer.install(candidate)

tests/test_installer.py:160:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../BUILDROOT/python-pdm-2.5.3-2.fc39.x86_64/usr/lib/python3.8/site-packages/pdm/installers/manager.py:33: in install
    installer(str(prepared.build()), self.environment, prepared.direct_url())
../../BUILDROOT/python-pdm-2.5.3-2.fc39.x86_64/usr/lib/python3.8/site-packages/pdm/installers/installers.py:195: in install_wheel_with_cache
    _install_wheel(wheel=wheel, destination=destination)
../../BUILDROOT/python-pdm-2.5.3-2.fc39.x86_64/usr/lib/python3.8/site-packages/pdm/installers/installers.py:268: in _install_wheel
    install(source, destination, additional_metadata=additional_metadata or {})
/usr/lib/python3.8/site-packages/installer/_core.py:109: in install
    record = destination.write_file(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pdm.installers.installers.InstallDestination object at 0x7f298317c2e0>
scheme = 'scripts', path = 'jp.py', stream = <zipfile.ZipExtFile [closed]>
is_executable = True

    def write_file(
        self,
        scheme: Scheme,
        path: Union[str, "os.PathLike[str]"],
        stream: BinaryIO,
        is_executable: bool,
    ) -> RecordEntry:
        """Write a file to correct ``path`` within the ``scheme``.

        :param scheme: scheme to write the file in (like "purelib", "platlib" etc).
        :param path: path within that scheme
        :param stream: contents of the file
        :param is_executable: whether the file should be made executable

        - Changes the shebang for files in the "scripts" scheme.
        - Uses :py:meth:`SchemeDictionaryDestination.write_to_fs` for the
          filesystem interaction.
        """
        path_ = os.fspath(path)

        if scheme == "scripts":
            with fix_shebang(stream, self.interpreter) as stream_with_different_shebang:
>               return self.write_to_fs(
                    scheme, path_, stream_with_different_shebang, is_executable, True
                )
E               TypeError: write_to_fs() takes 5 positional arguments but 6 were given

/usr/lib/python3.8/site-packages/installer/destinations.py:204: TypeError
---------------------------- Captured stdout setup -----------------------------
Changes are written to pyproject.toml.
Changes are written to pyproject.toml.
------------------------------ Captured log call -------------------------------
INFO     unearth.preparer:preparer.py:309 Downloading <Link http://fixtures.test/artifacts/jmespath-0.10.0-py2.py3-none-any.whl (from None)> (size unknown)
INFO     pdm.termui:installers.py:188 Installing wheel into cached location /tmp/pytest-of-tkloczko/pytest-123/test_install_wheel_with_data_s1/caches/packages/jmespath-0.10.0-py2.py3-none-any
=========================== short test summary info ============================
So as @pradyunsg wrote this fix is incorrect.

@pradyunsg
Copy link
Member

pradyunsg commented May 6, 2023

If that is true IMO it means that wheel module has the issue because it allows form .whl archive which such clash and it should detect that such situations and end with error.
Am I right?

I'm not sure if it's wheel or setuptools or something else.

The underlying reasoning for this issue is that the wheel is composed inappropriately. How and why that's been created is something I'm not going to be investigating. 😅

@wuch-g2v
Copy link

wuch-g2v commented May 6, 2023

IMO it is wheel bug because it should not generate .whl end exit with non-zero exit code if it is overlapping script and [console_scripts] like it is now in pecan
https://github.com/pecan/pecan/blob/cdb385ae8418f94b17be7b4ac1763a1ae7984b40/setup.py#L85-L100

PS. I must sent PR to fix that in pecan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants