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

Remove localpaths.py #10888

Open
jgraham opened this issue May 8, 2018 · 5 comments
Open

Remove localpaths.py #10888

jgraham opened this issue May 8, 2018 · 5 comments

Comments

@jgraham
Copy link
Contributor

jgraham commented May 8, 2018

Localpaths was more or less added so that people didn't have to worry about installing virtualenv and pip, and installing packages. But these days we have the wpt frontend, and it doesn't make sense to use sys.path hacks when we could instead just pip install -e all the components. This would have the advantage of being way less confusing.

One thing to be careful of is that this is likley to impact downstream consumers like Gecko, Servo and maybe also Blink/WebKit. With that in mind cc: @jdm, @Hexcles

@jgraham jgraham added the infra label May 8, 2018
@gsnedders
Copy link
Member

gsnedders commented May 8, 2018

Does it actually make sense to use -e? It probably makes it slightly easier given changes (but wpt can always deal with that), but it does still make it slightly odd (and more likely to work in spite of hardcoding paths we shouldn't).

@Hexcles
Copy link
Member

Hexcles commented May 8, 2018

@jgraham Did you mention some downsides of localpaths.py other than "it doesn't make sense" somewhere else (probably on IRC)? We have a few things that "don't make sense", so more context regarding the rationales / priorities would be great.

As far as the Blink infra goes, we are already doing some PYTHONPATH hacking before starting wptserve anyways (e.g. we don't use the pywebsocket in tools/third_party), and we don't use wpt frontend. Hence, if localpaths.py is removed, we will pretty much do the same path hacking in the our wptserve wrapper instead of relying on wpt or using pip install -e.

As for the upstream project itself, I too also have the same question as @gsnedders : is pip install -e the right tool for this? Forgive my limited experience, but I haven't this usage anywhere else.

@jgraham
Copy link
Contributor Author

jgraham commented May 8, 2018

I think that the localpaths thing creates as many problems as it solves. For example gecko was using the wptserve module for some tests, and when wptserve was changed to depend on localpaths this all broke because it wasn't really an independent component. Except that in reality it broke /later/ when some imports were moved around so that the bits depending on the rest of wpt ended up being loaded eagerly. So it wasn't even clear why it regressed when it did.

In general. Python packaging and modules are a mess. But we are making things gratuitously hard for ourselves by relying on modifying sys.path and putting things in just the right order that the paths are set up correctly at runtime, rather than having a package install step followed by the normal module import machinery.

FWIW I"m sure this will require some tweaking to work on the gecko side too, so I'm not proposing this as a way of making my life easy, but as a way of eliminating some technical debt that's no longer necessary.

@gsnedders
Copy link
Member

gsnedders commented May 9, 2018

As far as the Blink infra goes, we are already doing some PYTHONPATH hacking before starting wptserve anyways (e.g. we don't use the pywebsocket in tools/third_party)

Note we've somewhat forked pywebsocket, so I'd strongly recommend using the in-tree copy. (We'll probably end up forking it more given I think there's tests we can't write against it currently. And if we ever start supporting Py3 obviously that'll be a bigger fork.)

See also #10922.

@foolip
Copy link
Member

foolip commented May 6, 2021

web-platform-tests/rfcs#82 is relevant, and an approach like #28796 would lead to localpaths.py being removed, ultimately.

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

No branches or pull requests

4 participants