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

Maybe change METADATA.toml "requires"? #8312

Open
hauntsaninja opened this issue Jul 15, 2022 · 8 comments
Open

Maybe change METADATA.toml "requires"? #8312

hauntsaninja opened this issue Jul 15, 2022 · 8 comments
Assignees
Labels
project: infrastructure typeshed build, test, documentation, or distribution related

Comments

@hauntsaninja
Copy link
Collaborator

The context is I'm looking into #5768.

There's a lot of logic we have that either assumes or checks that entries in "requires" are typeshed stub packages. It seems potentially useful to split "requires" into "requires_typeshed" (a list of required packages that originate from typeshed) and "requires_external" (a list of external dependencies). Doing so could e.g. help avoid issues where we have typos in some requires_typeshed deps or potentially implicitly trust packages that start with "types-". Could also make it easier to build tooling (e.g. knowing what we could put on MYPYPATH to test against latest typeshed).

@srittau
Copy link
Collaborator

srittau commented Sep 18, 2022

I'd prefer to keep just one requires field. We ship METADATA.toml with the types package in question and for external users the source of the dependency should not matter and seems rather artificial. For internal purposes it seems rather trivial to check whether a dependency is internal.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Sep 19, 2022

I checked on grep.app and I think we're the only people who use requires, so not sure we should worry much about external users. I guess how much tooling and security benefit this would bring is debatable, but at the minimum this would be nice typo protection.

Anyway, your opinion should be the default here, so I won't push for this change. Are you on board with the rest of typeshed-internal/stub_uploader#59, assuming requires_typeshed becomes a derived property computed from filtering the requires field of METADATA.toml? I think adding something to the Metadata class is the sanest way to make sure all the old call sites that assume all deps come from typeshed only see typeshed deps. If not, I'll just close it out

@srittau
Copy link
Collaborator

srittau commented Sep 19, 2022

I checked on grep.app and I think we're the only people who use requires, so not sure we should worry much about external users.

I agree that this is more a theoretical concern at the moment.

Anyway, your opinion should be the default here, so I won't push for this change.

Well, your suggestion got a lot of upvotes, so it's quite possible I'm in the minority here, and it's also not an issue I'll fight tooth and nail over. So, I'd be interested what other people think.

Are you on board with the rest of typeshed-internal/stub_uploader#59

While I haven't reviewed the PR in detail yet, because of the issues under discussion, the general idea seems sound to me. And it definitely makes sense to distinguish between "internal" and "external" dependencies.

@AlexWaygood
Copy link
Member

I'd be interested what other people think.

I like @hauntsaninja's idea best.

If we adopted @Akuli's idea in #5768 (comment), it would reduce the concern regarding typos, since we could fairly easily assert that all dependencies either have names starting with types-, or are explicitly listed as dependencies of the runtime package in question. However, I still prefer @hauntsaninja's proposal. There's a tonne of places scattered across our test suite where we assume for various reasons that all packages listed in requires are internal typeshed packages. I like the simplicity of having the typeshed dependencies listed separately, so we don't have to separate out the typeshed dependencies from the non-typeshed dependencies in every test script.

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 20, 2022

It also feels slightly more "principled", to me, to list the typeshed-internal dependencies separately, rather than assuming that all types-* packages originate with typeshed (unless types-* names are reserved for typeshed somehow?)

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Sep 20, 2022

To be clear, we should never make assumptions based on the types- prefix. I assume srittau's suggestion was to partition requires based off of listing the stubs directory in typeshed or checking https://github.com/typeshed-internal/stub_uploader/blob/main/data/uploaded_packages.txt in stub_uploader.

Fwiw, the two differ in two cases: stubs permanently removed from typeshed and stubs newly added to typeshed. In stub_uploader we should probably only use uploaded_packages.txt to avoid sketchy things like race conditions where someone adds something to typeshed then squats the distribution before stub_uploader uploads it or bugs caused by things happening out of order when stubs are deleted (e.g. if we continue to have types-cryptography deps after deleting cryptography stubs).

@AlexWaygood
Copy link
Member

@hauntsaninja, are you still interested in pursuing this, or shall we close this for now?

@hauntsaninja
Copy link
Collaborator Author

I think this is still worth doing, especially so now that we're starting to have typeshed packages that aren't just types-.

(Of course, stub_uploader will still need to validate, like it does today, and if we have any typeshed CI workflows with write permissions we'd also need to validate)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: infrastructure typeshed build, test, documentation, or distribution related
3 participants