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

Run stubtest on stubs on different platforms #8923

Merged
merged 31 commits into from
Nov 11, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Oct 18, 2022

Open problems:

  • How to install deps on other platforms. brew works well on macos, but no idea how to use windows (I've never touched it since 2008)
  • Listing platform specific packages. Which ones should we add?
  • I still need to change daily.yml to do the same thing for all stubs
  • How to test this? Not quite clear for now. I will need to tweak some values

Refs #8660

@sobolevn
Copy link
Member Author

I have no idea how to fix this Windows problem:


Run STUBS=$(
ParserError: D:\a\_temp\4a20ead6-174f-4640-96c9-b47d87c97584.ps1:8
Line |
   8 |  if [ -n "$STUBS" ]; then
     |    ~
     | Missing '(' after 'if' in if statement.

It is valid sh, what more do you need? 🤣
Probably we need two different test runners: one for win and one for posix

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 18, 2022

It is valid sh, what more do you need? 🤣
Probably we need two different test runners: one for win and one for posix

The Windows shell is a whole different beast.

Can we use WSL in GitHub Actions? That might be an easy workaround.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator

Avasam commented Oct 18, 2022

It is valid sh, what more do you need?

But not valid powershell ! Although you already figured out that you can specify the shell in github actions. Which is what I would've recommended over:

Can we use WSL in GitHub Actions? That might be an easy workaround.


How to install deps on other platforms

Chocolatey is what I see most often on windows (at least when developing with Node and Python).
Brew for MacOS.


Listing platform specific packages. Which ones should we add?

Whatever's needed as you develop/test this I guess?  
Taking a look at stubtest_allowlist.txt files, and skip=true in METADATA.toml, seems to be a good starting point to get an idea of which stubs have os-specific needs.

@Akuli
Copy link
Collaborator

Akuli commented Oct 19, 2022

You can use shell: bash on Windows github actions.

@sobolevn
Copy link
Member Author

I already do that, thanks!

@AlexWaygood
Copy link
Member

Could you make this change to the METADATA.toml file for D3DShot as part of this PR, to test that the Windows part of this is working?

  [tool.stubtest]
- # The library only works on Windows; we currently only run stubtest on Ubuntu for third-party stubs in CI.
- # See #8660
- skip = true
+ platform = ["win32"]

@Akuli
Copy link
Collaborator

Akuli commented Oct 19, 2022

Nit: I would prefer if the metadata key was named platforms instead of platform.

@AlexWaygood
Copy link
Member

You might have to add a @tests/requirements-stubtest.txt file to D3DShot, and pin Pillow to a specific revision, in order to get it to install.

@sobolevn
Copy link
Member Author

Output from windows daily run: https://github.com/sobolevn/typeshed/actions/runs/3280905801/jobs/5402262235

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member Author

Since we now have platform-specific stubtest failures: https://github.com/sobolevn/typeshed/actions/runs/3283716889/jobs/5408788907

I will introduce platform-specific allowlists. Something similar to what we have in stdlib/

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks for all the work you've done on this!

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight wording suggestion because many stubs may be "supported" on other platforms, we just don't need to run stubtest multiple times.

Co-authored-by: Jelle Zijlstra <[email protected]>
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more tiny nits I spotted while looking over again

.github/workflows/stubtest_third_party.yml Outdated Show resolved Hide resolved
.github/workflows/stubtest_third_party.yml Outdated Show resolved Hide resolved
.github/workflows/stubtest_third_party.yml Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -2,6 +2,6 @@ version = "0.1.*"
requires = ["types-Pillow"]

[tool.stubtest]
# The library only works on Windows; we currently only run stubtest on Ubuntu for third-party stubs in CI.
# See #8660
# TODO: re-enable
Copy link
Member Author

@sobolevn sobolevn Nov 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# TODO: re-enable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this TODO comment I did like -- it would be good if we could figure out how to enable stubtest for this package!

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood
Copy link
Member

Time for this to go in.

@Avasam
Copy link
Collaborator

Avasam commented Nov 12, 2022

@sobolevn & @AlexWaygood There's an unforseen issue: Now I can't run stubtest locally on any stub that doesn't explicitely specify my non-linux OS!

If no platform is specified in METADATA.toml, it should be allowed to be run on any OS, but the CI should only run linux.

@sobolevn
Copy link
Member Author

I think we can add --local-run flag to be sure that it is indeed runing on a local env.

@Avasam
Copy link
Collaborator

Avasam commented Nov 12, 2022

Proposed solution: #9173

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

Successfully merging this pull request may close these issues.

5 participants