Windows support #640

nkaretnikov · 2023-10-22T14:38:26Z

See #507. Original PR: #559.

The main.js file from conda-store-ui contains an emoji character that cannot be encoded correctly unless the encoding is set to utf-8, which is not always the default on Windows.

Windows uses a different type of permissions system than Unix. For now, using None will cause action_set_conda_prefix_permissions to do nothing.

1000 hits the sqlite3 max expression depth and results in an error.

This disables --beat on Windows and sets FORKED_BY_MULTIPROCESSING=1, which is required to make the default celery pool work on Windows.

os.path.join is incorrect for constructing a URL on Windows because URLs should always use forward slashes regardless of platform.

The cookie length was incorrectly using .seconds instead of .total_seconds(). This was only a problem on Windows because Windows has a ~16 ms precision for timedeltas, meaning the .seconds attribute for the difference was sometimes being set to 0.

This reverts commit d753976.

This is needed for uses with --json, because otherwise any stderr breaks the json.

conda-store-server/tests/test_actions.py

nkaretnikov · 2023-10-22T17:22:32Z

Note: Windows test takes 25 min. macOS: 14 min. Ubuntu: 10 min.

I wonder why that is. E.g., the post set up env on Windows takes 3 mins, while it takes 4 secs on macOS. Is this disk/fs perf? Other phases are also slower by 2 mins on average.

conda-store-server/conda_store_server/app.py

conda-store-server/conda_store_server/schema.py

conda-store-server/conda_store_server/action/base.py

conda-store-server/tests/test_actions.py

nkaretnikov · 2023-10-22T19:20:17Z

conda-store-server/tests/test_usage.py

+    test_file2.write_text("b"*1000)
+    # Test hard links
+    test_file_hardlink = test_dir / "test_file_hardlink"
+    test_file_hardlink.hardlink_to(test_file)


Note for reviewers: this is tested where test_dir size is tested as a whole.

nkaretnikov · 2023-10-22T19:45:51Z

conda-store-server/conda_store_server/action/base.py

            encoding="utf-8",
        )
        self.stdout.write(result.stdout)
+        if not redirect_stderr:
+            self.stderr.write(result.stderr)
        return result


See #559 (comment) for context on redirects.

nkaretnikov · 2023-10-22T19:50:08Z

conda-store-server/conda_store_server/server/auth.py

-            max_age=(authentication_token.exp - datetime.datetime.utcnow()).seconds,
+            max_age=int(
+                (authentication_token.exp - datetime.datetime.utcnow()).total_seconds()
+            ),


See https://github.com/conda-incubator/conda-store/pull/559/files#r1364084772 for context on this.

A fun bug 😄 glad we got to the bottom of this one.

nkaretnikov · 2023-10-22T21:28:41Z

docs/user_guide.md

@@ -73,6 +73,10 @@ conda-unpack

 ### Docker Registry

+```{note}


Not sure if {note} syntax is supported. It seems to be ignored if not, so seems harmless.

nkaretnikov · 2023-10-22T22:25:49Z

Manually tested this env via the admin UI:

channels:
- conda-forge
dependencies:
- python
- pip:
  - nothing
- ipykernel
- pytest
- requests
description: ''
name: test-env
prefix: null
variables: null

Building, rebuilding, and deletion work.

nkaretnikov · 2023-10-22T22:27:14Z

@costrouc This is Aaron's Windows PR with some minor changes from me. PTAL and merge if OK.

jaimergp · 2023-10-23T07:50:37Z

the post set up env on Windows takes 3 mins

You can do run-post: false (or similar name) on setup-miniconda. It's an ephemeral VM after all.

costrouc

Overall these changes do not look controversial and I'm surprised that the windows fixes were actually smaller than I expected. I was really expecting issues with pathlib etc but it looks like most of those bugs didn't exist.

costrouc · 2023-10-24T16:11:13Z

conda-store-server/conda_store_server/orm.py

@@ -513,10 +513,11 @@ def update_packages(self, db, subdirs=None):
                package_builds[package_key].append(new_package_build_dict)
            logger.info("CondaPackageBuild objects created")

-            batch_size = 1000
+            # sqlite3 has a max expression depth of 1000
+            batch_size = 990


I bet there is an interesting story here? Is this only something that showed up on Windows?

I haven't seen this personally, but it's mentioned in one of the Aaron's commits:

Lower the batch size for updating packages from 1000 to 990 1000 hits the sqlite3 max expression depth and results in an error.

Seems harmless, so I decided to keep it.

costrouc · 2023-10-24T16:11:55Z

conda-store-server/conda_store_server/app.py

@@ -302,21 +302,24 @@ def _default_celery_results_backend(self):
    )

    default_uid = Integer(
-        os.getuid(),
+        None if sys.platform == "win32" else os.getuid(),


I see sys.platform == "win32" in many places. Should we have a constant that we create for this? Since much of our code only has a special path for Windows?

More I think about it I don't think this is necessary to have a constant

There was a constant in Aaron's original code. Well, two constants, in different files. Since you need to share this between multiple files, you'd have to put it in __init__ to avoid import issues. But we don't do this for any other platforms, so I decided to simply use sys.platform, as everywhere else, to avoid introducing new entities.

costrouc · 2023-10-24T16:13:24Z

conda-store-server/conda_store_server/server/app.py

-            openapi_url=os.path.join(self.url_prefix, "openapi.json"),
-            docs_url=os.path.join(self.url_prefix, "docs"),
-            redoc_url=os.path.join(self.url_prefix, "redoc"),
+            openapi_url=posixpath.join(self.url_prefix, "openapi.json"),


I'll admit using os.path... for urls is a hack and I should have used something like yarl for example. Thoughts? Since url paths are not the same as file paths? Maybe posixpath is the right thing to use here. It certainly seems better than os.path.

A separate entity for URLs would be better, yes. But I'm not going to block this PR just because of this issue, since everything seems to work fine.

costrouc · 2023-10-24T16:14:10Z

conda-store-server/conda_store_server/server/auth.py

-            max_age=(authentication_token.exp - datetime.datetime.utcnow()).seconds,
+            max_age=int(
+                (authentication_token.exp - datetime.datetime.utcnow()).total_seconds()
+            ),


A fun bug 😄 glad we got to the bottom of this one.

costrouc · 2023-10-24T16:19:29Z

conda-store-server/conda_store_server/utils.py

        cmd = ["du", "-sb", str(path)]
+    else:
+        return str(du(path))


Good to see the fallback is this approach 👍 If they are using cygwin or https://learn.microsoft.com/en-us/sysinternals/downloads/du it may still be possible they have du.

We've already looked at du from sysinternals. Besides being a separate tool, it also produces slightly different results. And the output format is also different. As for cygwin, the context for this work is to be able to use native Windows tooling.

costrouc · 2023-10-24T16:20:21Z

conda-store-server/conda_store_server/utils.py

+    output = subprocess.check_output(cmd, encoding="utf-8").split()[0]
+    if sys.platform == "darwin":
+        # mac du does not have the -b option to return bytes
+        output = str(int(output) * 512)


Why 512? This is a weird size since it is a half a kb.

A comment from a discussion on the original PR: "Note that the numbers will be off a little bit (by less than 512) on Mac because du rounds up to the nearest multiple of 512 when converting bytes to blocks."

I'm still not certain that macOS code is correct here, but it does pass this new test (where we check all three platforms), so I'm going to keep this and will take a look later.

costrouc · 2023-10-24T16:21:25Z

conda-store-server/conda_store_server/worker/app.py

        ]

+        # The default Celery pool requires this on Windows. See
+        # https://stackoverflow.com/questions/37255548/how-to-run-celery-on-windows


Great glad this fix was minimal.

It's still not clear whether it means that Celery works as intended, since it's not officially supported on Windows. But it does run, so we'll just use this for now.

costrouc · 2023-10-24T16:24:00Z

conda-store-server/tests/test_usage.py

+    val = disk_usage(test_dir)
+    assert isinstance(val, str)
+    assert 2000 + dir_size <= int(val) <= 2700 + dir_size
+    assert 2000 + dir_size <= du(test_dir) <= 2700 + dir_size


The exact values depend on the block size and other details

Oh yeah this is a fun detail 😄 guess you can't be smaller than the blocksize and also can only take up an integer number of block sizes.

asmeurer added 30 commits October 22, 2023 14:03

Fix pip install -e . on Windows

73d6bd1

The main.js file from conda-store-ui contains an emoji character that cannot be encoded correctly unless the encoding is set to utf-8, which is not always the default on Windows.

Use None for permissions when on Windows

cea6670

Windows uses a different type of permissions system than Unix. For now, using None will cause action_set_conda_prefix_permissions to do nothing.

Use the Python API for alembic rather than a subprocess

a7b3338

Use python -m conda_lock

d2d0f29

Lower the batch size for updating packages from 1000 to 990

0dc444c

1000 hits the sqlite3 max expression depth and results in an error.

Use a pure Python equivalent of du on Windows

58fd321

Fix missing parenthesis

48787eb

Fix the worker for Windows

99e65ae

This disables --beat on Windows and sets FORKED_BY_MULTIPROCESSING=1, which is required to make the default celery pool work on Windows.

Use posixpath to construct URLs

cb2dffb

os.path.join is incorrect for constructing a URL on Windows because URLs should always use forward slashes regardless of platform.

Fix test_action_decorator to work on Windows

3f4b340

Fix test_action_decorator on Windows

f334d31

Skip test_set_conda_prefix_permissions on Windows

1107907

Fix du() on Mac and Windows to return bytes instead of blocks

7dd17f1

Add Windows to CI

129b8b0

Run pre-commit

9e1ddef

Try using python -m on CI

eaf6d20

Revert "Try using python -m on CI"

0bf528a

This reverts commit d753976.

Trigger CI

e610a5a

Run tests in verbose mode

5b395c6

Add an option to not redirect stderr in context.run

ea643d7

This is needed for uses with --json, because otherwise any stderr breaks the json.

Don't capture stderr with conda env export --json

b908389

Print conda-store server address to the terminal when using --standalone

c3d1327

Use "localhost" instead of "127.0.0.1" for consistency with the docs

af302b8

Add a note to the docs that Docker image creation only works on Linux

362266c

Document that filesystem permissions options aren't supported on Windows

71d015c

Fix a formatting issue in the docs

5913460

Don't document a config file for using --standalone

370bb8b

Add FAQ entry for long paths on Windows

90b6650

Add a basic test for disk_usage()/du()

2b05f8f

Simplify du

a71f24d