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

Validate extras option in tox.ini #1113

Open
webknjaz opened this issue Jan 1, 2019 · 11 comments
Open

Validate extras option in tox.ini #1113

webknjaz opened this issue Jan 1, 2019 · 11 comments
Labels
area:configuration feature:new something does not exist yet, but should help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@webknjaz
Copy link
Contributor

webknjaz commented Jan 1, 2019

Long story short, tox ignores obviously invalid extra names. If could alert users about wrongly formatted items.

Background: pypa/virtualenv#1276

@gaborbernat
Copy link
Member

gaborbernat commented Jan 1, 2019

To solve this we need to define what is obviously wrong: Standard defined at https://packaging.python.org/specifications/core-metadata/#provides-extra-multiple-use:

A Python identifier is a name used to identify a variable, function, class, module or other object. An identifier starts with a letter A to Z or a to z or an underscore (_) followed by zero or more letters, underscores and digits (0 to 9).

That being said I believe it's pip that should fail, as the one using it, not tox itself.

@gaborbernat gaborbernat added feature:new something does not exist yet, but should area:configuration help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. labels Jan 1, 2019
@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 1, 2019

Well, the current behaviour is that pip just prints a warning that there's no such extra in a dist and ignores it. That's be a breaking change..

@obestwalter
Copy link
Member

I don't think we should behave differently from the tool that we're wrapping (in this case pip), so if the tool doesn't fail, tox shouldn't either. This is also for the sake of making reasoning about what should be happening easier, because I can always try out what happens, if I use the wrapped tool directly with what I am trying to configure in tox.

@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 4, 2019

I understand your position, but tox is a testing tool. It creates a wrong impression that everything went well.

It is important that it'd alert the user that something is wrong exactly because it's especially wrong to be misleading during the testing process.

@obestwalter
Copy link
Member

obestwalter commented Jan 5, 2019

It creates a wrong impression that everything went well.

In this concrete example where I mistype the name of the extras to be installed, I would expect the process to fail later anyway because needed packages are not installed and then I'll have to find out why. That's a pain and it would be nicer if it would have failed earlier, but this is something that should be decided in the packaging tool - in this case: pip. I had a look in the pip issues and can't find an open issues about this yet. If you open one (or find the one I didn't find), please let us know here.

Also: I'd rather say it passes the wrong impression truthfully along :)

On a more general note: The thought that tox abstracts the problems and weaknesses of the underlying tools away and even enhances on their actual behaviour is very tempting and a reoccuring theme in quite a few issues, but I am growing more opposed to this general idea as tempting as it might be for a number of reasons that I won't go into detail about, but the main reason is:

all abstractions are leaky

All is nice if things work as they should, but if things do not work they need to be debugged on the level where the problem occurs.

So in our case, if I want to install a set of extras and this does not happen, then I need to find out why pip (or any other packaging tool I might be using) doesn't install it. For this I need knowledge about how this tool behaves.

In this case we would need to run tox with higher verbosity to see all output of the underlying tools and then we stumble over the warning emitted by pip.

The burden of this can't be taken away by tox and the more diverse the packaging scene in Python will get over the next years the more of a step back tox needs to take there.

This is why I would propose to close this (and similar) requests as wontfix.

P.S. (off topic, but need to add my 2 cents :))

tox is a testing tool

To me tox is a generic automation tool. Orchestrating tests is just one of the many things it can do.

P.P.S

If I mistype the name of a key (e.g. extra) then tox does not crash either, but this should happen IMO. I opened an issue for that: #1121

@gaborbernat
Copy link
Member

I myself would agree it's a pip gap, so let's implemented it there (if we can't we can add it here). As for #1121 that option I don't think it's doable either without breaking a lot of current code, so overall, not worth it. In general, if people miss-type configuration the runtime will usually fail. In virtualenvs case it did not fail only because pip was already provided in a different way, would it be another package would have failed the environment at runtime, reporting it does not have the missing dependencies.

@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 5, 2019

In this concrete example where I mistype the name of the extras to be installed, I would expect the process to fail later anyway because needed packages are not installed and then I'll have to find out why. That's a pain

This is exactly what happened to me wrt #1097: because of a series of unrelated errors it has lead me into the wrong direction and caused a lot of misleading assumptions resulting in a lot of time being wasted.
So that's what caused me to have this opinion. I'm not opposed to it being fixed in pip but at least a warning in tox would be nice.

So in our case, if I want to install a set of extras and this does not happen, then I need to find out why pip [...] doesn't install it

Sometimes it tests have this thing "test x if y is available" and we don't know that something doesn't happen for a while and your whole "investigate this" flow stops there, it just never starts.
"y" hasn't been installed because of mistyped extra installation was silently ignored, test "x" got skipped, full test suite outcome is green and the fact that test is never invoked gets discovered months later and code which it covered turns out to be buggy because of delusion that it was being tested except it wasn't...

You can say that one would notice that coverage reduced but that's not always the case. Especially when you inherit a project from someone else who missed it...

P.S. Yes, tox is more than just testing tool. But in the context of this issue, I'd like to treat it like it.

@obestwalter
Copy link
Member

obestwalter commented Jan 5, 2019

This is exactly what happened to me wrt #1097: because of a series of unrelated errors it has lead me into the wrong direction and caused a lot of misleading assumptions resulting in a lot of time being wasted. So that's what caused me to have this opinion.

Ah, I understand. That's annoying indeed :( - this is why in general I am also a big proponent of the idea that a process should crash at the earliest possible time, but in our case here it is sadly not that easy to determine.

I'm not opposed to it being fixed in pip but at least a warning in tox would be nice.

Would you have a suggestion how this could be done in a reasonably sane way?

@webknjaz
Copy link
Contributor Author

webknjaz commented Jan 5, 2019

Following this https://packaging.python.org/specifications/core-metadata/#provides-extra-multiple-use it should be easy to classify absolutely invalid values. In the case of dist building enabled for env it should be quite easy to read the produced metadata which explicitly lists all of its extras.

@obestwalter
Copy link
Member

In the light of this, it might be a worthy workaround after all 🤔

@obestwalter
Copy link
Member

obestwalter commented Jan 5, 2019

But ... it wouldn't solve the problem when using a valid identifier that happens to be not a valid name, so it would only be a partial solution (and the complete solution could only be implemented inside pip in a feasible way).

@gaborbernat gaborbernat added this to the 4.1 milestone Jan 13, 2022
@gaborbernat gaborbernat removed this from the P-1 milestone Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration feature:new something does not exist yet, but should help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.
Projects
None yet
Development

No branches or pull requests

3 participants