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

Support Windows... somewhat #1734

Closed
wants to merge 8 commits into from

Conversation

wyattscarpenter
Copy link
Contributor

@wyattscarpenter wyattscarpenter commented Aug 11, 2024

In order to support windows, we need to use the c++-standard named struct params like @HexDecimal suggested in #1154 (comment) and change cxx_std=11 to 20 like @rchen152 suggested in #1154 (comment).

There may be more changes required. In particular, I am getting one test failing in the Tests step1 of the CI:

    File "D:\a/pytype/pytype/out\pytype\io_test.py", line 169, in test_unused_imports_info_files
      self.assertEqual(0, ret)
  AssertionError: 0 != 1
    ⋮
   FAILED: pytype/io_test.log D:/a/pytype/pytype/out/pytype/io_test.log
  C:\Windows\system32\cmd.exe /C "cd /D D:\a\pytype\pytype\out\pytype && C:\Python\Lib\site-packages\cmake\data\bin\cmake.exe -E env TYPESHED_HOME=D:/a/pytype/pytype/typeshed C:/Python/python.exe -B D:/a/pytype/pytype/build_scripts/test_module.py pytype.io_test -o D:/a/pytype/pytype/out/pytype/io_test.log -P D:/a/pytype/pytype/out -s -p"
  C:\Users\runneradmin\AppData\Local\Temp\tmp2coltc1x\m.py:1:1: error: in <module>: Can't find module 'common.foo'. [import-error]
  
  from common import foo; print(foo.x)~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I don't know why. But this is still an incremental improvement in Windows support for pytype.

Footnotes

  1. Additionally, in my repo, I've changed the CI script to run all of the checks and not early-out; in this, the Windows version now passes the Extensions Tests and gets some error in Type Check.

this involves: 1. using the c++-standard named struct params like @HexDecimal suggested in google#1154 (comment) ; 2. changing  cxx_std=11 to 20 like @rchen152 suggested in google#1154 (comment)
…should work cross-platform

this requires using a higher version of CMake, but actually our CMakeLists.txt already has a cmake_minimum_required(VERSION 3.16)
@wyattscarpenter wyattscarpenter changed the title Support Windows... maybe Support Windows... somewhat Aug 11, 2024
Copy link
Member

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

It's a nice change

requirements.txt Outdated
@@ -2,6 +2,7 @@
# Make sure you also install the non-Python dependencies described in
# https://github.com/google/pytype/blob/master/CONTRIBUTING.md#pytype-dependencies.
attrs>=21.4.0
cmake>=3.16
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't be adding cmake here.

  1. As I understand, it's a build-only dependency, whereas this file lists run-time dependencies. It is not necessary to install cmake to use pytype (I think)

  2. It is rather controversial to install cmake from PyPI, or at least I haven't heard of this before.

  3. For GitHub Actions it shouldn't be a problem, cmake is always available there

Choose a reason for hiding this comment

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

I agree that cmake should at least be moved to build-system.requires in pyproject.toml, which already includes tools such as ninja.

I'd consider cmake and ninja to be equally controversial, being build tools external to Python's ecosystem, and ninja is already being installed from PyPI here.

Copy link
Member

Choose a reason for hiding this comment

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

I see, indeed they are about equal. Sure.

Oh and also this file is for dev dependencies, we are not adding it to install dependencies.

OK, this is fine.

Choose a reason for hiding this comment

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

My mistake. If it's only a dev dependency than it should be mentioned in CONTRIBUTING.md instead, which CMake already is. I'd agree with removing it from the requirements.txt in this case.

The CMake version in CONTRIBUTING.md should be bumped to 3.16 since that's the current minimum required version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Luckily, I already bumped the CMake version in CONTRIBUTING.md to 3.16! :)

I don't 100% understand which of these dependencies are used when, but moving cmake to build-system.requires works (moving ninja out of requirements.txt in the same way doesn't also work), so I have done so. Uh, by "works" I mean the CI fails on the same further step, of course.

Choose a reason for hiding this comment

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

requirements.txt: suggested dependencies for package developers, in this case the ones working on pytype directly, would be installed into your local venv of your cloned project, sometimes automatically installed by your IDE.

build-system.requires: dependencies required to run setup.py or whichever build-system.build-backend is specified (currently setup.py), but are not needed to import the package normally. Do not move a dependency into build-system.requires unless the Python build step fails because that dependency is missing.

I haven't touched this project since #1154, having multiple suggested ways to build the project is a little confusing. pip install -e . would use build-system.requires, but the non-standard python build_scripts/build.py would expect the dependencies from requirements.txt instead. You are going to get different results depending on which build method is being used. I'm too much of an outsider to give any suggestions or advice on this, and my earlier advice may have been premature.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, I think I'm not ready to accept these changes to build requirements within this pull request.

The decision how exactly cmake gets pulled in does not directly affect Windows support, and the situation is not unique to Windows; the support can be achieved without changing this. But it does affect every system in ways that we haven't fully understood yet.

So in the interest of getting this merged more easily, I am going to revert those changes from this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Thanks!

oprypin
oprypin previously approved these changes Aug 13, 2024
copybara-service bot pushed a commit that referenced this pull request Aug 14, 2024
In order to support windows, we need to use the c++-standard named struct params like @HexDecimal suggested in #1154 (comment) and change cxx_std=11 to 20 like @rchen152 suggested in #1154 (comment).

There may be more changes required. In particular, I am getting *one* test failing in the Tests step[^1] of the CI:
```
    File "D:\a/pytype/pytype/out\pytype\io_test.py", line 169, in test_unused_imports_info_files
      self.assertEqual(0, ret)
  AssertionError: 0 != 1
    ⋮
   FAILED: pytype/io_test.log D:/a/pytype/pytype/out/pytype/io_test.log
  C:\Windows\system32\cmd.exe /C "cd /D D:\a\pytype\pytype\out\pytype && C:\Python\Lib\site-packages\cmake\data\bin\cmake.exe -E env TYPESHED_HOME=D:/a/pytype/pytype/typeshed C:/Python/python.exe -B D:/a/pytype/pytype/build_scripts/test_module.py pytype.io_test -o D:/a/pytype/pytype/out/pytype/io_test.log -P D:/a/pytype/pytype/out -s -p"
  C:\Users\runneradmin\AppData\Local\Temp\tmp2coltc1x\m.py:1:1: error: in <module>: Can't find module 'common.foo'. [import-error]

  from common import foo; print(foo.x)~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ```
 I don't know why. But this is still an incremental improvement in Windows support for pytype.

 [^1]: Additionally, in my [repo](https://github.com/wyattscarpenter/pytype/actions/runs/10342876412/jo[]#step:6:289), I've changed the CI script to run all of the checks and not early-out; in this, the Windows version now passes the Extensions Tests and gets some error in Type Check.

PiperOrigin-RevId: 662898874
@oprypin
Copy link
Member

oprypin commented Aug 14, 2024

Merged as 53135b9. Thank you

@oprypin oprypin closed this Aug 14, 2024
@wyattscarpenter wyattscarpenter deleted the patch-1 branch August 14, 2024 19:22
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.

3 participants