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 unused setup.py files in tools/ #33318

Closed
wants to merge 1 commit into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Mar 23, 2022

Clean up webdriver/README.md since it referenced the setup.py being
removed.

tools/wptrunner/setup.py still remains and is needed to run tox tests,
but can probably be simplified or removed somehow.

Clean up webdriver/README.md since it referenced the setup.py being
removed.

tools/wptrunner/setup.py still remains and is needed to run tox tests,
but can probably be simplified or removed somehow.
@foolip foolip marked this pull request as ready for review March 23, 2022 16:54
@foolip
Copy link
Member Author

foolip commented Mar 23, 2022

OK, turns out one can remove tools/wptrunner/setup.py by also adding skipsdist=True in tools/wptrunner/tox.ini. @jgraham could this have some effect on Gecko, where you use wptrunner without the ./wpt frontend?

@gsnedders
Copy link
Member

This is a much broader change, which potentially impacts how we deal with the difference packages and whether we want them to be meaningful packages, along with how we deal with the tooling infrastructure as a whole. I'd much rather we actually changed what is/isn't a package, and how things get imported, in a much more coherent way than doing something like this.

Removing the setup.py from wptrunner while still having it deeply nested feels odd. Like, why would I do from tools.wptrunner import wptrunner if we're moving in that direction, when we're doing from tools import manifest?

@foolip
Copy link
Member Author

foolip commented Mar 24, 2022

@gsnedders are your comments only about tools/wptrunner/setup.py (which I've left alone in this PR) or also about tools/webdriver/setup.py and tools/wptserve/setup.py?

@jgraham
Copy link
Contributor

jgraham commented Mar 24, 2022

We have previously done some releases of wptserve, and it's used outside of web-platform-tests in a few cases. The webdriver library isn't as far as I know but it's at least theoretically a reusable component and I think it makes sense to maintain that position since it makes it clear where the abstraction boundaries should lie.

So I think this PR is unsafe to land as is (because it will prevent doing more wptserve releases) and I agree with @gsnedders that this kind of change should motivated by an overall strategy for how we handle our internal dependencies going forward.

@foolip
Copy link
Member Author

foolip commented Mar 24, 2022

Is there something we can do to clarify the purpose of these setup.py files and ensure they actually work for that purpose? The reason I came across these now is because we use setuptools, which I was searching for while looking at #33309.

It looks like at least tools/wptrunner/setup.py is broken, since it refers to files that no longer exist, like testharness_marionette.js which was removed in fce5996.

I agree with @gsnedders that this kind of change should motivated by an overall strategy for how we handle our internal dependencies going forward.

Do you mean everything discussed in web-platform-tests/rfcs#82? Or just that we should have an RFC to say we will never do new releases of wptrunner or wptserve before making that impossible?

@jgraham
Copy link
Contributor

jgraham commented Mar 24, 2022

Do you mean everything discussed in web-platform-tests/rfcs#82? Or just that we should have an RFC to say we will never do new releases of wptrunner or wptserve before making that impossible?

More like web-platform-tests/rfcs#82

It's not at all clear to me that "we will never do another release of a subcomponent as a package" is the right policy.

@foolip
Copy link
Member Author

foolip commented Mar 24, 2022

Now I'm confused :) web-platform-tests/rfcs#82 is the thing I linked to, but AFAICT we didn't talk about our internal packages there...

@jgraham
Copy link
Contributor

jgraham commented Mar 24, 2022

Right, I really meant "like" in the sense of "similar to", i.e. I think the decision making here should be driven by a shared understanding of our requirements/goals, which probably involves a new RFC.

@foolip
Copy link
Member Author

foolip commented Mar 25, 2022

Maybe we can make this all much simpler by just testing this in CI. How does setup.py ever end up being run? I suspect they're broken, but is there a command I can put in CI to ensure they actually work? That and a comment at the top would solve the issue for me.

@jgraham
Copy link
Contributor

jgraham commented Mar 25, 2022

Yeah, you could have a CI command that does something like python3 setup.py sdist in the relevant directories and check that it returns a zero exit status. I just tried manually and they seem to still work as expected.

@foolip
Copy link
Member Author

foolip commented Mar 25, 2022

Thanks @jgraham! That must mean that the testharness_marionette.js bit is either ignored or harmless. I'll look into exercising these files in CI.

@foolip foolip marked this pull request as draft March 25, 2022 17:15
@foolip foolip closed this Oct 11, 2022
@foolip foolip deleted the rm-setup.py branch October 11, 2022 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants