Validate platforms #12245

MohsenAAkeel · 2023-08-26T04:41:32Z

Fixes #6369
This pr adds two bits of functionality:

 1. adds validation on user submitted platform tags
As suggested in issue #6369, I attempted to put the call to the validation function in the common base class to DownloadCommand and InstallCommand, namely the Command class. This resulted in a multitude of test failures. As a result, the call to the function was added to each of the DownloadCommand and InstallCommand run methods. The functions that perform the validation are added to cmdoptions.py because this seemed like the most logical place to have them. You'll note that first a function named validate_user_options is called. This is because additional functionality (as discussed in the issue) will be called through this (specifically to check implementation and abi user options). Though the issue calls for an 'error' message, I attempted to add a warning. This proved to be problematic because none of the tests use standard option inputs and were not configured to accept warnings. Instead I use a simple print command with the message.

There is a second pr that will be either already up (or up soon) that adds validation for --implementation and --python-version.

Regarding testing. I added a test for this in the test_download.py file. I ran the entire tests directory with nox and all passed save for 13 errors (which were unrelated to the code in this PR and exist in the latest main branch). In other words, the code in this PR passes.

2. extends platform help text to include suggestions for valid platform tags
 I noticed that some other options (e.g. implementation and python_version) had help text that provided examples, but that the help text for platform did not. I went ahead and added some common examples.

cclauss

Some simplifications.

src/pip/_internal/cli/cmdoptions.py

fail fast simplifications, legibility, some performance improvement Co-authored-by: Christian Clauss <cclauss@me.com>

…and and formatting adjustments removing unnecessary vars removing unused libs removing unused libs adding List type hint ruff ordering of imports making platform check console prints and adding check to install command nox format fixes updating platform option check test to match updates adjusting news nox format fixes updating platform option check test to match updates adding suggestions to invalid platform input warning changing platform check function to catchall user option check adding validation for platforms options removing validation to incorporate in separate branch adding news file adding news file fixed re fullmatch method call raw re strings adjusting formatting adjusting formatting formatting adjustments adding function return types adjusting text formatting adding test for platform validation warning message adding validation of platform options from cmdoptions adding platform option validation and extending platform help text

chrysle · 2023-08-27T13:59:57Z

src/pip/_internal/cli/cmdoptions.py

+            f"{current_platform} and {current_machine}"
+        )
+
+        print(message)


I think you should use write_output() here.

chrysle · 2023-08-27T14:09:05Z

src/pip/_internal/cli/cmdoptions.py

+        if platform_prefix == "macosx":
+            if not is_macos_arch(platform_suffix):
+                invalid_platforms.append(platform_tag)
+        elif platform_prefix in {
+            "linux",
+            "manylinux1",
+            "manylinux2010",
+            "manylinux2014",
+        }:
+            if not is_linux_arch(platform_prefix, platform_suffix):
+                invalid_platforms.append(platform_tag)
+        elif platform_prefix == "manylinux":
+            if not is_glibc_linux_arch(platform_suffix):
+                invalid_platforms.append(platform_tag)
+        elif platform_prefix in ["win", "win32"]:
+            if not is_win_arch(platform_suffix):
+                invalid_platforms.append(platform_tag)


I find it unnested easier to read.

Suggested change

if platform_prefix == "macosx":

if not is_macos_arch(platform_suffix):

invalid_platforms.append(platform_tag)

elif platform_prefix in {

"linux",

"manylinux1",

"manylinux2010",

"manylinux2014",

}:

if not is_linux_arch(platform_prefix, platform_suffix):

invalid_platforms.append(platform_tag)

elif platform_prefix == "manylinux":

if not is_glibc_linux_arch(platform_suffix):

invalid_platforms.append(platform_tag)

elif platform_prefix in ["win", "win32"]:

if not is_win_arch(platform_suffix):

invalid_platforms.append(platform_tag)

if platform_prefix == "macosx" and not is_macos_arch(platform_suffix):

invalid_platforms.append(platform_tag)

elif platform_prefix in {

"linux",

"manylinux1",

"manylinux2010",

"manylinux2014",

} and not is_linux_arch(platform_prefix, platform_suffix):

invalid_platforms.append(platform_tag)

elif platform_prefix == "manylinux" and not is_glibc_linux_arch(platform_suffix):

invalid_platforms.append(platform_tag)

elif platform_prefix in ["win", "win32"] and not is_win_arch(platform_suffix):

invalid_platforms.append(platform_tag)

chrysle · 2023-08-27T14:13:12Z

src/pip/_internal/cli/cmdoptions.py

+        if prefix == "linux":
+            return arch in {"i386", "x86_64"}
+        if prefix in {"manylinux1", "manylinux2010"}:
+            return arch in {"i686", "x86_64"}
+        if prefix == "manylinux2014":
+            return arch in {
+                "aarch64",
+                "armv71",
+                "i686",
+                "ppc64",
+                "ppc64le",
+                "s390x",
+                "x86_64",
+            }


For consistency.

Suggested change

if prefix == "linux":

return arch in {"i386", "x86_64"}

if prefix in {"manylinux1", "manylinux2010"}:

return arch in {"i686", "x86_64"}

if prefix == "manylinux2014":

return arch in {

"aarch64",

"armv71",

"i686",

"ppc64",

"ppc64le",

"s390x",

"x86_64",

}

if prefix == "linux":

return arch in {"i386", "x86_64"}

elif prefix in {"manylinux1", "manylinux2010"}:

return arch in {"i686", "x86_64"}

elif prefix == "manylinux2014":

return arch in {

"aarch64",

"armv71",

"i686",

"ppc64",

"ppc64le",

"s390x",

"x86_64",

}

https://beta.ruff.rs/docs/rules/superfluous-else-return I disliked this rule when I first saw it but I have gotten used to it.

pfmoore · 2023-08-27T15:13:51Z

IMO, it's wrong to present this as "validating platforms". There is no definition of what constitutes a valid platform string - the spec doesn't define a set of valid values, and in fact explicitly allows anything that distutils.util.get_platform could return (and in fact, the spec probably needs updating, as distutils is no longer part of the stdlib, so using it to define a standard is not ideal).

At best this is a PR to check that the supplied value is plausible. Which is not an unreasonable thing to do but:

I'm concerned that this would require ongoing maintenance as new platforms are supported by Python.
It's not clear how to tell what the valid values for such a new platform might be without simply trying it out.
It's a lot of code for a pretty niche feature.

So I'm at best ambivalent and tending towards apposed to this change. Apart from anything else, it doesn't actually address #6369, because the request there is to know what the valid values are, which isn't answerable given the current standards.

I'd rather see the spec for compatibility tags tightened up to either make it easier for users to work out the correct tags for a system they don't necessarily have access to, or clarify that the correct way to find a correct set of tags is to query the target system (using something like packaging.tags.platform_tags()).

Short of modifying the specification, I'd rather address #6369 by simply changing pip's documentation of --platform to say that to find the correct value to use, you should call packaging.tags.platform_tags() on the target system.

chrysle · 2023-08-27T16:02:08Z

I'd rather see the spec for compatibility tags tightened up to either make it easier for users to work out the correct tags for a system they don't necessarily have access to, or clarify that the correct way to find a correct set of tags is to query the target system (using something like packaging.tags.platform_tags()).

Crosslink

MohsenAAkeel · 2023-08-27T18:39:16Z

@pfmoore

Just to clarify - from the original comment point 2 was meant to address the issue as posted and point 1 to address a member comment regarding adding user feedback on input

I've not much of a background on the topic and as I was working on this I did note there were no explicit standards given so I agree this makes the language used in the PR inaccurate. That said, at the risk of appearing too attached to the PR, might something like this be useful at the point of error logging? Specifically, moving these checks to package_finder.py in method find_requirements (ln 935) where errors for unmet requirements are given? Admittedly this does not resolve the maintenance issue, but it does make this less 'froward facing' in that uncommon or new platforms that do find a match will not trigger it. If this seems like too much overhead for its value, happy to nix this part of the pr.

For clarification on the --platform help text: you prefer to replace the suggestions as given in this PR to language directing users to use packaging.tags.platform_tags(), or both?

As for revisiting the spec document fleshed out, I could have a go at it while everything I've learned is still rattling around in my head. Anything you had in mind specifically to see?

pfmoore · 2023-08-27T19:11:44Z

The big problem is that I simply don't think there's a need for a list of valid values. If we assume that --platform is intended to allow the user to specify the appropriate values for a target system they know and have access to, then telling them to get the correct value by querying the target is sufficient.

Providing any sort of "list of valid values" suggests that guessing which value applies to your target platform is a valid approach, and honestly I don't believe it is - it might work, but it might also just result in additional user frustration and errors.

So I guess what I'm saying is that I'm -1 on this because it's a lot of code, providing an unreliable hint on how to use the flag in a way that I think isn't the best way to use it. However, it is also true that I don't use (or need) the platform option myself, so my intuition on how it should be handled is largely theoretical. If any of the other @pypa/pip-committers has practical experience with this option, and feels like this is a good approach, I'm happy to defer to them.

MohsenAAkeel added 24 commits August 22, 2023 21:01

adding platform option validation and extending platform help text

efba1c4

adding validation of platform options from cmdoptions

6f15f37

adding test for platform validation warning message

3219d46

adjusting text formatting

fee41cf

adding function return types

b5073e8

formatting adjustments

c5e42e6

adjusting formatting

b125ebb

adjusting formatting

84bcc9e

raw re strings

f9fe710

fixed re fullmatch method call

aad3bb8

adding news file

5172723

adding news file

613e888

removing validation to incorporate in separate branch

8528792

adding validation for platforms options

938b537

changing platform check function to catchall user option check

74db805

adding suggestions to invalid platform input warning

f36daa6

updating platform option check test to match updates

c3e779a

nox format fixes

1bf164a

adjusting news

24b3f39

Merge branch 'main' into validate_platforms

036a853

updating platform option check test to match updates

647d283

nox format fixes

ba1814c

making platform check console prints and adding check to install command

35e5ff7

removing unnecessary vars

4e0e873

psf-chronographer bot added the bot:chronographer:provided label Aug 26, 2023

MohsenAAkeel added 5 commits August 26, 2023 00:53

removing unused libs

f6304dc

removing unused libs

8320a5a

adding List type hint

3cecf26

nox formatting adjustments

f4d76df

ruff ordering of imports

1b626ca

cclauss reviewed Aug 26, 2023

View reviewed changes

MohsenAAkeel and others added 6 commits August 26, 2023 20:04

Apply suggestions from code review

306343a

fail fast simplifications, legibility, some performance improvement Co-authored-by: Christian Clauss <cclauss@me.com>

syntax error correction

0e964e3

syntax error correction

02af1cc

removing unused lib that go back in..

7d23a08

nox formatting

64fe181

chrysle reviewed Aug 27, 2023

View reviewed changes

pradyunsg removed the bot:chronographer:provided label Dec 20, 2023

psf-chronographer bot added the bot:chronographer:provided label Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Validate platforms #12245

Validate platforms #12245

Uh oh!

MohsenAAkeel commented Aug 26, 2023 •

edited

Loading

Uh oh!

cclauss left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrysle Aug 27, 2023

Uh oh!

chrysle Aug 27, 2023

Uh oh!

chrysle Aug 27, 2023

Uh oh!

cclauss Aug 27, 2023

Uh oh!

pfmoore commented Aug 27, 2023

Uh oh!

chrysle commented Aug 27, 2023

Uh oh!

MohsenAAkeel commented Aug 27, 2023 •

edited

Loading

Uh oh!

pfmoore commented Aug 27, 2023

Uh oh!

Uh oh!

Validate platforms #12245

Are you sure you want to change the base?

Validate platforms #12245

Uh oh!

Conversation

MohsenAAkeel commented Aug 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cclauss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrysle Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

chrysle Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

chrysle Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

cclauss Aug 27, 2023

Choose a reason for hiding this comment

Uh oh!

pfmoore commented Aug 27, 2023

Uh oh!

chrysle commented Aug 27, 2023

Uh oh!

MohsenAAkeel commented Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfmoore commented Aug 27, 2023

Uh oh!

Uh oh!

MohsenAAkeel commented Aug 26, 2023 •

edited

Loading

MohsenAAkeel commented Aug 27, 2023 •

edited

Loading