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

Update wheel.py to support generic tag when no extensions found #598

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bsdz
Copy link

@bsdz bsdz commented May 29, 2023

If no extensions are built (due to conditions) then do not tag as platform specific wheel.

Resolves: python-poetry/poetry#8039

  • Added tests for changed code.
  • Updated documentation for changed code.

If this PR looks reasonable I'm happy to add some documentation. I'd need pointers on where to add tests.

@sonarcloud
Copy link

sonarcloud bot commented May 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dimbleby
Copy link
Contributor

dimbleby commented May 29, 2023

Surely there are lots of other ways for platform-specific files to make their way into a wheel eg as scripts, or by being built out-of-band and copied around, or whatever. This change will break the wheel name for anyone who is relying on such things.

(Also as a matter of coding style: bool | None is a weird type for a field that can take two values, it wants to be a boolean - perhaps with a better name! And I don't know that str(boolean).lower() is really an improvement on "true" if boolean else "false").

Also: no testcase.

@bsdz
Copy link
Author

bsdz commented May 30, 2023

Surely there are lots of other ways for platform-specific files to make their way into a wheel eg as scripts, or by being built out-of-band and copied around, or whatever. This change will break the wheel name for anyone who is relying on such things.

I'm open to suggestions on fixing this use case. See the issue I raised alongside with a mypyc example (but also applies to cython).

(Also as a matter of coding style: bool | None is a weird type for a field that can take two values, it wants to be a boolean - perhaps with a better name! And I don't know that str(boolean).lower() is really an improvement on "true" if boolean else "false").

I disagree. It's quite common to have a tenary variable being true, false and "don't know".

Also: no testcase.

I added a comment to this effect and, now, given your comment suggests this PR is DoA perhaps writing a test case would have been futile in any case.

@dimbleby
Copy link
Contributor

Unfortunately it is easier to pick holes than to propose solutions. Building with extensions etc is an under-loved part of the code and may need more invasive changes to address this sort of thing.

... ternary variable ...

but this isn't a ternary variable: it never takes the value True.

@bsdz
Copy link
Author

bsdz commented May 30, 2023

... ternary variable ...

but this isn't a ternary variable: it never takes the value True.

It doesn't mean it's not a ternary variable. At the outset the variable is indeterminate as no test has been done to test if it "has libs" or not. Once it has been determined there are no libs then it is set to False. I could have added

                    if not libs:
                        # The result of building the extensions
                        # does not exist, this may due to conditional
                        # builds, so we assume that it's okay
                        self._has_libs = False
                        return
                   else:
                        self._has_libs = True

but there would be no point as the variable will never be tested for being True.

It's still a ternary variable.

However, I feel we are arguing over semantics here.

@dimbleby
Copy link
Contributor

dimbleby commented May 30, 2023

it's certainly easier to debate whether a variable that takes two values can properly be described as ternary, than it is to figure out how to achieve the goal of this MR!

never be tested for being True.

Exactly! This is what shows that bool | None is the wrong type. Invalid states should be unrepresentable: you have two valid states and should reflect that with a type that has two valid values.

@bsdz
Copy link
Author

bsdz commented May 30, 2023

Also as a matter of coding style: bool | None is a weird type for a field that can take two values..

Oh wow!, look its been used in this repo several times already as well as thousands more times in other prominent python projects.

@dimbleby
Copy link
Contributor

dimbleby commented May 30, 2023

I think we are talking past each other. Of course bool | None is not in itself a bad type - for cases where there are three distinct states that the code cares about.

At least in the first of those examples that you linked - I didn't go through them all! - None and True and False have different meanings.

In this MR there are only two states that the code cares about: "I have built and there are no libs" or the opposite of that. So a two-state type is appropriate.

People often do use bool | None, defaulting to None, and then treating None as false-y. They might just as well have used bool and defaulted to False.

Your code is the other way round: you might just as well have used bool and defaulted to True.

@bsdz
Copy link
Author

bsdz commented May 31, 2023

I've created a workaround plugin poetry-plugin-ignore-build-script.

poetry self add poetry-plugin-ignore-build-script
poetry build --ignore-build-script

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