env: validate PEP 508 specifiers before passing them as arguments to pip

[eli-schwartz]

Per the specification for pyproject.toml build-system.requires, these MUST be valid PEP 508 strings, and when checking to see if they are satisfied during --no-isolation builds, they will be parsed using packaging.requirements which checks this for free as a side effect of converting each string into a good format for checking against importlib.metadata dists.

But when building by default, they are just pip installed and pip supports many things that aren't just PEP 508 strings. Fix this by first parsing them with packaging.requirements just to get the error message.

[eli-schwartz]

What should the preferred UX of this be? Previously, this emitted:

ERROR Command '['/tmp/build-env-3atlhb3x/bin/python', '-Im', 'pip', 'install', '--use-pep517', '--no-warn-script-location', '-r', '/tmp/build-reqs-a3ytyjeu.txt']' returned non-zero exit status 1.

with a preceding traceback and a pip error:

ERROR: Invalid requirement: 'xxx' (from line 1 of /tmp/build-reqs-a3ytyjeu.txt)

With my change, it errors out with a packaging.requirements.Requirement exception and logs:

ERROR Expected package name at the start of dependency specifier
    _vendored/meson
    ^

or

ERROR Expected end or semicolon (after name and no valid version specifier)
    this is invalid
         ^

The tests are doing some finicky position-based output matching, trying to figure out what it even expects...

[henryiii]

The position based matching is annoying, you can just update it to whatever the new line numbers in the source are.

pip supports many things that aren't just PEP 508 strings.

Will this break any current workarounds? (I'm thinking of #651, which apparently doesn't work now, but just wondering if there's anything like that to worry about). If so, maybe a warning first then an error later would be better? (Even though we say non-PEP 508 strings are unsupported, someone might not know what they are doing is unsupported, so a phase out period is friendlier in that case). But if not, then no need.

What should the preferred UX of this be?

The above seems fine to me? Though maybe printing out where this occurs (build-system.requires in pyproject.toml would be helpful.

[henryiii]

Will this break any current workarounds?

From the linked issue:

But if you build with isolation disabled then it does check that all dependencies are installed

So this is expanding the "with -n but without -x" behavior to all runs.

I think this is fine, but what about only running this if -x is not passed? That way, someone could disable this check by adding -x if they do need the undefined behavior. That would mean the default is now safe, but someone could work around this if they know what they are doing and need to build a package with an invalid spec here.

[layday]

-x is for (not) checking whether all declared dependencies are installed. The fact that it parses requirement strings to do that is incidental. IMO if we want to check that requirement strings are well-formed we should do that universally, in the builder and not the env, so that the same guarantee will extend to -nx builds as well as custom envs.

[eli-schwartz]

I think this is fine, but what about only running this if -x is not passed? That way, someone could disable this check by adding -x if they do need the undefined behavior. That would mean the default is now safe, but someone could work around this if they know what they are doing and need to build a package with an invalid spec here.

I think this should already be the case -- what I modified here was the case of "no -n".

I'm indifferent to what happens when -x is passed, , what I want to prevent is the case where an invalid PEP 508 string is successfully installed by treating it as a filepath relative to pyproject.toml.

I could probably clarify that in the commit message...

[henryiii]

IMO if we want to check that requirement strings are well-formed we should do that universally

IMO, long term, validating requirement strings is probably fine, but I'm thinking backward compatible, if someone today puts "subdirectory/project" as a requirement string, they can build their project out of the box today, as long as they don't pass -n. I think the best way forward is to enable requirement checking by default, but only if -x is not passed (regardless of if -n is passed). That way people using "subdirectory/project" will be alerted that what they are doing is not supported, but they have an emergency workaround (pass -x) while they redesign what they are doing. After this is out for a few months, we could start checking all the time.

So the suggestion is:

-n	-x	Today	Suggested	Someday
❌	❌	Unchecked	Checked	Checked
❌	✅	Unchecked	Unchecked	Checked
✅	❌	Checked	Checked	Checked
✅	✅	Unchecked	Unchecked	Checked

[gaborbernat]

Seems it stalled, @eli-schwartz let me know if you want to pick it up again and we can reopen.