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

Fixes for install and docs targets when contributing on Windows #7282

Merged
merged 1 commit into from Aug 29, 2023

Conversation

redruin1
Copy link
Contributor

@redruin1 redruin1 commented Aug 29, 2023

Change Summary

Tweaks pyproject.toml and docs/plugins/main.py so that no errors occur when performing common dev tasks like installing, running tests, and building docs on Windows.

Install:

> pip install pipx
> pipx install pdm
> pipx install pre-commit
> make install
  ...
  ✔ Install ruff 0.0.285 successful
  ✔ Install mkdocs-material 9.2.3 successful
  ✔ Install sqlalchemy 1.4.0 successful
Retry failed jobs
  ✖ Install memray 1.9.1 failed

ERRORS:
add memray failed:
Traceback (most recent call last):
  ...
  File "<string>", line 276, in <module>
RuntimeError: memray does not support this platform (win32)

Building tests then of course fail because of the missing import:

> make
...
File ".\Python\pydantic\env\lib\site-packages\pytest_memray\plugin.py", line 22, in <module>
    from memray import AllocationRecord
ModuleNotFoundError: No module named 'memray'
.\Python\pydantic\env\lib\site-packages\coverage\control.py:860: CoverageWarning: No data was collected. (no-data-collected)
  self._warn("No data was collected.", slug="no-data-collected")
make: *** [test] Error 1

This can be fixed by simply excluding memray's installation if the installers platform is Windows. This means no memory profiling on Windows, but I would argue that's less important for minimal changes and can always be tested in CI. Not much Pydantic can do about it, in any case.

Making docs also breaks, but for a different reason:

> make docs
...
  File ".\Python\pydantic\docs\plugins\main.py", line 64, in add_changelog
    history = (PROJECT_ROOT / 'HISTORY.md').read_text()
  File ".\AppData\Local\Programs\Python\Python310\lib\pathlib.py", line 1135, in read_text
    return f.read()
  File ".\AppData\Local\Programs\Python\Python310\lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
UnicodeDecodeError: 'charmap' codec can't decode byte 0x9d in position 7310: character maps to <undefined>
make: *** [docs] Error 1

Windows python seems to not want to read the text in "utf-8" by default, which is somewhat bizarre even for Windows. I changed the read_text() and write_text() functions to specify encoding="utf-8", which should probably be done for consistency anyway.

After these changes, installing, formatting, linting, typechecking, testing, coverage, and building docs all work out of the box on Windows. There are still a few commands that fail (like test-examples), but this is less a total conversion PR (and would be much more complex if it was), but instead a pair of fixes so that somebody on Windows who's willing to install make could follow along with the contribution page.

Maybe the documentation page could now recommend to install make if you're on Windows? All the remaining documentation would be unchanged.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist (N/A)
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review

Selected Reviewer: @davidhewitt

@redruin1
Copy link
Contributor Author

Please review

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks!

@davidhewitt davidhewitt merged commit 966ea40 into pydantic:main Aug 29, 2023
49 checks passed
@redruin1 redruin1 deleted the windows-dev-toolchain-fixes branch August 29, 2023 17:38
@davidhewitt davidhewitt added the relnotes-ignore Omit this PR from the release notes. label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review relnotes-ignore Omit this PR from the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants