MacOS and Windows support #397

mknorps · 2023-11-20T12:19:17Z

Support for MacOS

To support MacOS it is enough to add macos-latest to the test matrix and allow for longer runs for tests that use --install-deps option.

Support for Windows

To support Windows we needed to:

change all paths to be system-agnostic (5843d1e)
change paths that referred to python executable, pip executable and site packages to work on all systems (36e957a, 8c4ecb9, d689171)
Adjust command line script that creates virtual environments for tests (73e1819)
Skip irrelevant tests (442db96, 3b5e3b6)
skip real-project tests as the test setup does not work well with Windows and we may consider adjusting the tests (03fcd44)
few other minor tweaks.

Issues

In the process of adjusting the code to support Windows, we discovered, that the real_projects tests setup is not suited for Windows. The fact that we use a separate virtual environment, which we construct from the command line makes it hard to work with Windows.

You may see example of such problem in this CI run. In this case, project The Algorithms-Python uses conditional requirement:

    "tensorflow; python_version < '3.11'",

This falls into undeclared dependencies on Windows for Python <=3.10. This behavior should be investigated in a separate ticket.

On Windows pip and Python executable are located under venv_dir\Scripts\. `pip_path` depends now on OS.

On Windows, Python virtual environment keeps the 3rd partiies libraries under `{venv_dir}\Lib\site-packages`. Python version is not mentioned anywhere in the directories names.

On windows, a CI runner does not have admin premissions, which are needed to delete the temporary file constructed with `NamedTemporaryFile`.

In this commit, we use Ptrhon standard library to provide sys.executable and os.devnull.

Some of the tests are irrelevant to Windows. These were mearked to be skipped when run on Windows machine. Added one test for Windows, wchich is skipped for a non-Windows run.

This issue originates from the way we run real project tests. I included more details in the PR description.

Make all paths in the code system-agnostic. They are either build with os.path.join() or with operations on `Path` object.

…on a system For Windows, there is several differences like remove command, creation of an empty file. We make sure that packages are installed in the new virtual environment by running python from this virtual environment with `-m pip install`

…ptions execution time.

On Windows only administrator can create symlinks. We will skip this test on windows as one of a low impact.

For Python Algorithms project

This confuses Windows...

On Windows the ordering of logs is different.

For tests and format. For other workflows ubuntu with fixed version is changed to `ubuntu-latest`.

mknorps · 2024-01-03T10:43:16Z

First and foremost: I'm really impressed with this PR! 🤩 Although I am surprised that people are already using FawltyDeps on Windows with some success, it is obvious that we still have quite a lot of POSIX assumptions in our code, especially under tests/, and I'm very happy with how you have tackled this: Your PR systematically fixes everything across our (by now, quite large) codebase.

I've done a first complete pass over the PR, and added (too many, probably 😅) comments. I wasn't sure if I'd get through everything today, so I posted them as I went along. Apologies for not batching them into a single review.

Despite the number of comments, I really don't have any issues with the approach you've taken here. Most of the comments are minor niggles like using sys.platform instead of platform.system or Path() instead of os.path.join().

One thing that is becoming really apparent in this PR is that our real_projects tests need some more serious rework to work well on Windows. I think disabling these on Windows is the right way to solve this, for now. Tackling that rewrite is best done in a separate PR.

Up to you, whether you want to clean up this PR by removing the other real_projects-related changes, or leave them in. I'm happy either way.

Than you @jherland for your review and insights.

I fixed two main things you asked for - using Path instead of os.path.join and sys.platform instead of platform.system.

I would leave the real-project related challenges to another PR. I tested the Python Algorithms project that was failing in the real project tests on a Windows machine and it works.

For Windows users, I think that the most important is that when FawltyDeps looks for a mapping, it checks also virtual environment available in Windows. That is the most significant improvement in my opinion for a regular user. For a developer, we made it possible with this PR to contribute to FawltyDeps when writing code on Windows thanks to the tests refactor 🎉

jherland

Still a few comments here and there, but I am happy for this to be merged after you've had a look. Feel free to push back if you feel I'm getting too nitpicky. You can always tell me to fix things myself in a later PR 😅

There's one unresolved comment from the previous round, about the delete flag to NamedTemporaryFile where I think we need to be careful to get things right. Otherwise, my comments and suggestions are mostly just matters of taste, and I'm happy to get this merged with or without addressing those.

fawltydeps/packages.py

fawltydeps/types.py

tests/test_cmdline.py

tests/test_dir_traversal.py

tests/test_local_env.py

tests/utils.py

tests/test_types.py

… the system requires it

Co-authored-by: Johan Herland <johan.herland@tweag.io>

Reorder package search conditions and simplify one for Windows to resemble one for POSIX more.

…n executable

This commit adds 'path_sets_equal' function that checks if <Paths> created of provided strings are equal or not

mknorps force-pushed the maria/macos-and-windows-support-explorations branch from ca281ac to d551f11 Compare November 20, 2023 12:36

mknorps force-pushed the maria/macos-and-windows-support-explorations branch 3 times, most recently from 52c2291 to 3904f1a Compare November 28, 2023 12:56

mknorps force-pushed the maria/macos-and-windows-support-explorations branch 4 times, most recently from d529d90 to 4deb134 Compare December 21, 2023 10:17

mknorps changed the base branch from main to 25-experiments December 21, 2023 10:18

mknorps changed the base branch from 25-experiments to main December 21, 2023 10:18

mknorps force-pushed the maria/macos-and-windows-support-explorations branch 3 times, most recently from 0bfbdef to 92ddf45 Compare December 21, 2023 13:43

mknorps added 17 commits December 21, 2023 15:29

Allow to use pip on Windows

36e957a

On Windows pip and Python executable are located under venv_dir\Scripts\. `pip_path` depends now on OS.

Allow to use virtual environment site-packages on Windows

8c4ecb9

On Windows, Python virtual environment keeps the 3rd partiies libraries under `{venv_dir}\Lib\site-packages`. Python version is not mentioned anywhere in the directories names.

Do not delete temporary files automatically for NamedTemporaryFile

9915676

On windows, a CI runner does not have admin premissions, which are needed to delete the temporary file constructed with `NamedTemporaryFile`.

Use system-agnostic command for running Python and devnull

d689171

In this commit, we use Ptrhon standard library to provide sys.executable and os.devnull.

Test local env for all systems

442db96

Some of the tests are irrelevant to Windows. These were mearked to be skipped when run on Windows machine. Added one test for Windows, wchich is skipped for a non-Windows run.

Add more information on the real projects run of integration tests.

294a473

Skip real project tests if run on Windows

03fcd44

This issue originates from the way we run real project tests. I included more details in the PR description.

Make paths system-agnostic

5843d1e

Make all paths in the code system-agnostic. They are either build with os.path.join() or with operations on `Path` object.

Set limits to maximum number of examples and length of test_cmdline_o…

4f1e2f8

…ptions execution time.

Skip test on Symlinks for Windows

3b5e3b6

On Windows only administrator can create symlinks. We will skip this test on windows as one of a low impact.

Remove a deadline for resolver tests where install_deps option is used

4b65302

Adjust settings test to how Windows displays text in the stdout

c77b8b7

Use a single quote notation of additional conditions of requirements

6bf2fec

For Python Algorithms project

Remove backslash at the and of source code rendering

e091d77

This confuses Windows...

Check first two array entries for logging to stdin

df57258

On Windows the ordering of logs is different.

Add MacOS and Windows to the CI matrix

8c55c30

For tests and format. For other workflows ubuntu with fixed version is changed to `ubuntu-latest`.

mknorps force-pushed the maria/macos-and-windows-support-explorations branch 3 times, most recently from 139a3a7 to 091abe9 Compare January 3, 2024 10:37

mknorps force-pushed the maria/macos-and-windows-support-explorations branch 2 times, most recently from 9e41731 to dadf267 Compare January 3, 2024 10:47

mknorps added 2 commits January 3, 2024 11:48

Fix linter

7357a88

Add a test case for a path with disc name on Windows

d0fb833

mknorps force-pushed the maria/macos-and-windows-support-explorations branch from dadf267 to d0fb833 Compare January 3, 2024 10:48

Fix duplicate

5363e33

mknorps requested a review from jherland January 3, 2024 12:33

jherland approved these changes Jan 3, 2024

View reviewed changes

jherland mentioned this pull request Jan 3, 2024

Refactor real_projects tests to support Windows #406

Closed

mknorps and others added 2 commits January 3, 2024 18:33

Fix test types. Use string as expected values and transform / -> \ if…

5932b06

… the system requires it

Apply suggestions from code review

14ce40c

Co-authored-by: Johan Herland <johan.herland@tweag.io>

mknorps force-pushed the maria/macos-and-windows-support-explorations branch 2 times, most recently from 0eb7945 to b62885f Compare January 4, 2024 12:17

Use unlink to remove temporary file. Trick needed for Windows support

f4b2a0a

mknorps force-pushed the maria/macos-and-windows-support-explorations branch from b62885f to f4b2a0a Compare January 4, 2024 12:23

mknorps added 2 commits January 4, 2024 15:32

Move site_packages to fawtydeps utils.

a5fb067

Reorder package search conditions and simplify one for Windows to resemble one for POSIX more.

Skip traverse project tests on Windows

a5af8be

mknorps force-pushed the maria/macos-and-windows-support-explorations branch 3 times, most recently from 31d3ba2 to fb17603 Compare January 8, 2024 10:05

mknorps added 2 commits January 8, 2024 12:06

Add condition for a virtual environment on Windows to contain a Pytho…

a5a6ad0

…n executable

Apply Johan's code review suggestions

e16b180

mknorps force-pushed the maria/macos-and-windows-support-explorations branch from fb17603 to e16b180 Compare January 8, 2024 11:07

Paths on Windows are equal regardless of capital/lower case

976911f

This commit adds 'path_sets_equal' function that checks if <Paths> created of provided strings are equal or not

mknorps merged commit 30cc11a into main Jan 8, 2024
65 checks passed

mknorps deleted the maria/macos-and-windows-support-explorations branch January 8, 2024 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MacOS and Windows support #397

MacOS and Windows support #397

mknorps commented Nov 20, 2023 •

edited

Loading

mknorps commented Jan 3, 2024 •

edited

Loading

jherland left a comment

MacOS and Windows support #397

MacOS and Windows support #397

Conversation

mknorps commented Nov 20, 2023 • edited Loading

Support for MacOS

Support for Windows

Issues

mknorps commented Jan 3, 2024 • edited Loading

jherland left a comment

Choose a reason for hiding this comment

mknorps commented Nov 20, 2023 •

edited

Loading

mknorps commented Jan 3, 2024 •

edited

Loading