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 #491

Open
5 tasks
ewjoachim opened this issue Nov 19, 2021 · 11 comments
Open
5 tasks

Support Windows #491

ewjoachim opened this issue Nov 19, 2021 · 11 comments
Labels
Issue appropriate for: People up for a challenge 🤯 This issue probably will be challenging to tackle Issue contains: Exploration & Design decisions 🧩 We don't know how this will be implemented yet Issue contains: Some Python 🐍 This issue involves writing some Python code Issue contains: Some Windows 🪟 This issue will require skills and/or testing capabilities under Windows Issue type: Feature ⭐️ Add a new feature that didn't exist before

Comments

@ewjoachim
Copy link
Member

  • Add windows tests in the CI
  • Check that signals work the same way
  • Check that envvars work the same way
  • Adjust documentation
  • Find a windows maintainer
@ewjoachim ewjoachim added Issue appropriate for: People up for a challenge 🤯 This issue probably will be challenging to tackle Issue contains: Exploration & Design decisions 🧩 We don't know how this will be implemented yet Issue contains: Some Python 🐍 This issue involves writing some Python code Issue contains: Some Windows 🪟 This issue will require skills and/or testing capabilities under Windows Issue type: Feature ⭐️ Add a new feature that didn't exist before labels Nov 19, 2021
@ewjoachim
Copy link
Member Author

Ping @aleksandr-shtaub

@aleksandr-shtaub
Copy link
Contributor

aleksandr-shtaub commented Nov 22, 2021

I'll start from the easy ones.

Env vars

Works as expected.

CMD

rem Set value for a variable Name. Note: Should not contain any whitespaces around `=`.
> set Name=Value

rem Get value of variable `Name`.
> echo %Name%
Value

or

PowerShell

# Set value for a variable Name. Note: powershell have types, so use `""` for a string type.
> $Env:Name = "Value"  

# Get value of variable.
> echo $Env:Name
Value

Example

# test.py
import os
print(os.environ.get("Greeter"))
> $Env:Greeter = "Hello!"
> python -m test
Hello!

Paths

Unix: a/b/c
Win: a\\b\\c

Consider using Path.asPosix() instead of str(Path) when testing paths.

Signals

signal.SIGINT, signal.SIGTERM are supported (kind of) (1)

Subprocess

Different resolving of executable path (2)

We should use full paths or shell=True when we use subprocess.Popen().
Popen is in tests so there is nothing to worry about.

Popen.send_signal(signal.SIGINT) is not supported on win (3)

# subprocess.py
...
    def send_signal(self, sig):
        """Send a signal to the process."""
        # Don't signal a process that we know has already died.
        if self.returncode is not None:
            return
        if sig == signal.SIGTERM:
            self.terminate()
        elif sig == signal.CTRL_C_EVENT:
            os.kill(self.pid, signal.CTRL_C_EVENT)
        elif sig == signal.CTRL_BREAK_EVENT:
            os.kill(self.pid, signal.CTRL_BREAK_EVENT)
        else:
            raise ValueError("Unsupported signal: {}".format(sig))
...

Event Loop

add_signal_handler() and remove_signal_handler() aren't supported on win (4)

ProactorEventLoop and SelectorEventLoop (5,6,7,8)

We cant' use ProactorEventLoop because add_reader() and remove_reader() are used by aiopg.
Since Python 3.8 we need to change default policy explicitly like:

if os.platform == "win32":
    asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())

Tests

Pytest-asyncio doesn't pickup policy stated in our code (9, 10)

We can rewrite fixture like this:

@pytest.fixture
def event_loop(request):
    """Rewrite event_loop fixture for pytest_asyncio."""
    if sys.platform == "win32":
        asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
    loop = asyncio.get_event_loop_policy().new_event_loop()
    yield loop
    loop.close()

TBC (if I find something else)

@aleksandr-shtaub
Copy link
Contributor

Related:
#286
#444
#451

@ewjoachim
Copy link
Member Author

@spapas
Copy link
Contributor

spapas commented Nov 4, 2024

Hello friends, can you please clarify if it is possible to run this project under windows? This isn't clear for me right now. I only care about a test/dev environment (my production is on linux). If it is possible can you recommend me how ?

Right now, when trying to run the worker under windows I'm getting the following error:

python manage.py procrastinate worker
error connecting in 'pool-1': Psycopg cannot use the 'ProactorEventLoop' to run in async mode. Please use a compatible event loop, for instance by setting 'asyncio.set_event_loop_policy(WindowsSelectorEventLoopPolicy())'

@ewjoachim
Copy link
Member Author

It's untested. If it works, it's mainly by chance, and if it doesn't but you see how to fix it, we accept pull requests. If anyone is willing to step up and maintain the lib under Windows (e.g. accept to be pinged whenever something comes up), I'll be happy to share the keys and call the project officially supporting Windows :)

I don't have a windows machine (well, not really).

@spapas
Copy link
Contributor

spapas commented Nov 5, 2024

I did some quick tests and found out that two changes are needed for this to run on windows (at least for dev):

  1. Add this line asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) before running the procrastinate cli
  2. Do not use signals in signals.py

1 is simple however I can't understand the implications of 2. Let's suppose we don't do anything on install and uninstall functions of signals.py what would happen? Can you ELI5 what's the purpose of these signal handlers?

TIA

@ewjoachim
Copy link
Member Author

Signals are used so that when the (Unix-based) OS informs procrastinate it needs to stop gracefully. Without signal, when you interrupt (ctrl+c, sigint) the program, or the process manager wants to stop it (sigterm), it would stop immediately, halfway through running jobs. Thanks to signals instead, we continue running but stop taking new jobs and halts when all running jobs are done (modulo various timeouts)

@spapas
Copy link
Contributor

spapas commented Nov 6, 2024

@ewjoachim thank youfor the explanation. As I understand this project can be used for development in Windows with very minor changes (apply the WindowsSelectorEventLoopPolicy and ignore signals). Would it be possible to implement that functionality in a new release?

@ewjoachim
Copy link
Member Author

Would it be possible to implement that functionality in a new release?

It would be possible, but this doesn't say who does it :)

I'm running my own contribution time on a limited schedule at the moment, so it's somehow complicated for me to commit on doing the PR, especially since I wouldn't be able to test that it works correctly. On the other hand, of you want to submit a PR, it will be much easier for me (or another maintainer) to review and merge it 😃

@spapas
Copy link
Contributor

spapas commented Nov 11, 2024

Hello @ewjoachim I understand the problem. If you are willing to accept such a PR, i.e no proper windows support but support for development under windows I'd be happy to test the needed changes on my development environment and provide a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue appropriate for: People up for a challenge 🤯 This issue probably will be challenging to tackle Issue contains: Exploration & Design decisions 🧩 We don't know how this will be implemented yet Issue contains: Some Python 🐍 This issue involves writing some Python code Issue contains: Some Windows 🪟 This issue will require skills and/or testing capabilities under Windows Issue type: Feature ⭐️ Add a new feature that didn't exist before
Projects
None yet
Development

No branches or pull requests

3 participants