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

Windows support #559

Closed
wants to merge 53 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
b4e70b6
Fix pip install -e . on Windows
asmeurer Sep 5, 2023
1719638
Use None for permissions when on Windows
asmeurer Sep 6, 2023
322afb5
Use the Python API for alembic rather than a subprocess
asmeurer Sep 6, 2023
f8f7ce4
Use python -m conda_lock
asmeurer Sep 12, 2023
8cf9e0b
Lower the batch size for updating packages from 1000 to 990
asmeurer Sep 15, 2023
8eb15dd
Merge branch 'main' into windows
asmeurer Sep 15, 2023
76ce349
Use a pure Python equivalent of du on Windows
asmeurer Sep 18, 2023
1f095cb
Merge branch 'main' into windows
asmeurer Sep 25, 2023
abb13ed
Fix missing parenthesis
asmeurer Sep 25, 2023
cf4951e
Fix the worker for Windows
asmeurer Sep 25, 2023
80d9ea8
Use posixpath to construct URLs
asmeurer Sep 25, 2023
877f816
Fix login not working consistently on Windows
asmeurer Oct 5, 2023
59c40db
Fix test_action_decorator to work on Windows
asmeurer Oct 5, 2023
e5fb32c
Fix test_action_decorator on Windows
asmeurer Oct 5, 2023
d87ad02
Skip test_set_conda_prefix_permissions on Windows
asmeurer Oct 5, 2023
d612642
Fix du() on Mac and Windows to return bytes instead of blocks
asmeurer Oct 5, 2023
c08690b
Merge branch 'main' into windows
asmeurer Oct 9, 2023
17d3b3a
Add Windows to CI
asmeurer Oct 9, 2023
be633b3
Run pre-commit
asmeurer Oct 9, 2023
d753976
Try using python -m on CI
asmeurer Oct 9, 2023
a06679c
Revert "Try using python -m on CI"
asmeurer Oct 9, 2023
e639e22
Try activating environment base on Windows
asmeurer Oct 9, 2023
00f193a
Remove mamba from linux CI too
asmeurer Oct 9, 2023
36c99d3
Try using channel-priority: strict
asmeurer Oct 10, 2023
b7b0cc6
CI test
asmeurer Oct 10, 2023
07dc9e8
Trigger CI
asmeurer Oct 10, 2023
dfc3f5b
Try using only conda-forge on Windows
asmeurer Oct 10, 2023
dab4a66
Try using miniforge
asmeurer Oct 10, 2023
c036b27
Set conda-forge as the only channel in ~/.condarc
asmeurer Oct 10, 2023
0c65c05
Fix command
asmeurer Oct 10, 2023
780753a
Fix shell command
asmeurer Oct 10, 2023
98ba0e9
Try force reinstalling Python on Windows
asmeurer Oct 10, 2023
52f25c7
Use the correct environment
asmeurer Oct 10, 2023
b492c27
Revert "Use the correct environment"
asmeurer Oct 10, 2023
26bc094
Add a comment
asmeurer Oct 10, 2023
58c25ec
Try removing extra setup-miniconda config
asmeurer Oct 10, 2023
cd66987
Run tests in verbose mode
asmeurer Oct 10, 2023
1afb0fc
Add an option to not redirect stderr in context.run
asmeurer Oct 10, 2023
5d647ea
Don't capture stderr with conda env export --json
asmeurer Oct 10, 2023
c20d612
Disable mamba in the integration tests too
asmeurer Oct 10, 2023
2d7c953
Print conda-store server address to the terminal when using --standalone
asmeurer Oct 11, 2023
d759fe9
Use "localhost" instead of "127.0.0.1" for consistency with the docs
asmeurer Oct 11, 2023
871f219
Add a note to the docs that Docker image creation only works on Linux
asmeurer Oct 11, 2023
5a3ef3a
Document that filesystem permissions options aren't supported on Windows
asmeurer Oct 11, 2023
e083cc8
Fix a formatting issue in the docs
asmeurer Oct 11, 2023
4918a08
Don't document a config file for using --standalone
asmeurer Oct 11, 2023
9bb4317
Add FAQ entry for long paths on Windows
asmeurer Oct 11, 2023
d47c588
Add a basic test for disk_usage()/du()
asmeurer Oct 11, 2023
d863809
Run black
asmeurer Oct 11, 2023
d993d43
Document that there are different environment files for Mac and Windows
asmeurer Oct 11, 2023
99ec7b2
Fix filenames
asmeurer Oct 11, 2023
eb31b84
Fix filename
asmeurer Oct 11, 2023
2216f47
Account for the size of the directory itself (which is large on Linux)
asmeurer Oct 12, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ jobs:
name: "unit-test conda-store-server"
strategy:
matrix:
# cannot run on windows due to needing fake-chroot for conda-docker
os: [ubuntu-latest, macos-latest]
os: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
defaults:
run:
Expand All @@ -29,7 +28,6 @@ jobs:
if: matrix.os == 'ubuntu-latest'
uses: conda-incubator/setup-miniconda@v2
with:
mamba-version: "*"
activate-environment: conda-store-server-dev
environment-file: conda-store-server/environment-dev.yaml
auto-activate-base: false
Expand All @@ -42,6 +40,24 @@ jobs:
environment-file: conda-store-server/environment-macos-dev.yaml
auto-activate-base: false

- name: Set up Python (Windows)
if: matrix.os == 'windows-latest'
uses: conda-incubator/setup-miniconda@v2
with:
activate-environment: conda-store-server-dev
environment-file: conda-store-server/environment-windows-dev.yaml
auto-activate-base: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up: #634 switches to miniforge. This will need to be updated to match the rest of the jobs once that PR lands.


# This fixes a "DLL not found" issue importing ctypes from the hatch env
- name: Reinstall Python 3.10 on Windows runner
uses: nick-fields/[email protected]
with:
timeout_minutes: 9999
nkaretnikov marked this conversation as resolved.
Show resolved Hide resolved
max_attempts: 6
command:
conda install --channel=conda-forge --quiet --yes python=${{ matrix.python }}
if: matrix.os == 'windows-latest'

- name: Linting Checks
run: |
hatch env run -e dev lint
Expand All @@ -52,7 +68,7 @@ jobs:

- name: Unit Tests
run: |
pytest tests
pytest -vvv tests
nkaretnikov marked this conversation as resolved.
Show resolved Hide resolved

integration-test-conda-store-server:
name: "integration-test conda-store-server"
Expand All @@ -68,7 +84,6 @@ jobs:
- name: Set up Python
uses: conda-incubator/setup-miniconda@v2
with:
mamba-version: "*"
activate-environment: conda-store-server-dev
environment-file: conda-store-server/environment-dev.yaml
auto-activate-base: false
Expand Down
9 changes: 6 additions & 3 deletions conda-store-server/conda_store_server/action/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def wrapper(*args, **kwargs):
# redirect stdout -> action_context.stdout
stack.enter_context(contextlib.redirect_stdout(action_context.stdout))

# redirect stderr -> action_context.stdout
# redirect stderr -> action_context.stderr
Copy link
Contributor

Choose a reason for hiding this comment

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

The line below uses action_context.stdout, so this change is wrong. Or the line below needs to be updated.

stack.enter_context(contextlib.redirect_stderr(action_context.stdout))

# create a temporary directory
Expand All @@ -38,20 +38,23 @@ class ActionContext:
def __init__(self):
self.id = str(uuid.uuid4())
self.stdout = io.StringIO()
self.stderr = io.StringIO()
self.log = logging.getLogger(f"conda_store_server.action.{self.id}")
self.log.propagate = False
self.log.addHandler(logging.StreamHandler(stream=self.stdout))
self.log.setLevel(logging.INFO)
self.result = None
self.artifacts = {}

def run(self, *args, **kwargs):
def run(self, *args, redirect_stderr=True, **kwargs):
result = subprocess.run(
*args,
**kwargs,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
stderr=subprocess.STDOUT if redirect_stderr else subprocess.PIPE,
encoding="utf-8",
)
self.stdout.write(result.stdout)
if not redirect_stderr:
self.stderr.write(result.stderr)
nkaretnikov marked this conversation as resolved.
Show resolved Hide resolved
return result
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,7 @@ def action_generate_conda_export(
"--json",
]

result = context.run(command, check=True)
result = context.run(command, check=True, redirect_stderr=False)
if result.stderr:
context.log.warning(f"conda env export stderr: {result.stderr}")
nkaretnikov marked this conversation as resolved.
Show resolved Hide resolved
return json.loads(result.stdout)
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import json
import pathlib
import sys
import typing

from conda_store_server import action
Expand All @@ -16,7 +17,9 @@ def action_install_lockfile(
json.dump(conda_lock_spec, f)

command = [
"conda-lock",
sys.executable,
"-m",
"conda_lock",
"install",
"--validate-platform",
"--log-level",
Expand Down
11 changes: 8 additions & 3 deletions conda-store-server/conda_store_server/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
)
from traitlets.config import LoggingConfigurable

ON_WIN = sys.platform.startswith("win")


def conda_store_validate_specification(
db: Session,
Expand Down Expand Up @@ -301,21 +303,24 @@ def _default_celery_results_backend(self):
)

default_uid = Integer(
os.getuid(),
None if ON_WIN else os.getuid(),
help="default uid to assign to built environments",
config=True,
allow_none=True,
)

default_gid = Integer(
os.getgid(),
None if ON_WIN else os.getgid(),
help="default gid to assign to built environments",
config=True,
allow_none=True,
)

default_permissions = Unicode(
"775",
None if ON_WIN else "775",
help="default file permissions to assign to built environments",
config=True,
allow_none=True,
)

default_docker_base_image = Union(
Expand Down
7 changes: 4 additions & 3 deletions conda-store-server/conda_store_server/dbutil.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import os
from contextlib import contextmanager
from subprocess import check_call
from tempfile import TemporaryDirectory

from alembic import command
Expand Down Expand Up @@ -78,6 +77,8 @@ def upgrade(db_url, revision="head"):
current_table_names = set(inspect(engine).get_table_names())

with _temp_alembic_ini(db_url) as alembic_ini:
alembic_cfg = Config(alembic_ini)

if (
"alembic_version" not in current_table_names
and len(current_table_names) > 0
Expand All @@ -86,10 +87,10 @@ def upgrade(db_url, revision="head"):
# we stamp the revision at the first one, that introduces the alembic revisions.
# I chose the leave the revision number hardcoded as it's not something
# dynamic, not something we want to change, and tightly related to the codebase
command.stamp(Config(alembic_ini), "48be4072fe58")
command.stamp(alembic_cfg, "48be4072fe58")
# After this point, whatever is in the database, Alembic will
# believe it's at the first revision. If there are more upgrades/migrations
# to run, they'll be at the next step :

# run the upgrade.
check_call(["alembic", "-c", alembic_ini, "upgrade", revision])
command.upgrade(config=alembic_cfg, revision=revision)
5 changes: 3 additions & 2 deletions conda-store-server/conda_store_server/orm.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
all_package_keys = list(package_builds.keys())
for i in range(0, len(all_package_keys), batch_size):
logger.info(f"handling subset at index {i} (batch size {batch_size}")
logger.info(f"handling subset at index {i} (batch size {batch_size})")
subset_keys = all_package_keys[i : i + batch_size]

# retrieve the parent packages for the subset
Expand Down
14 changes: 8 additions & 6 deletions conda-store-server/conda_store_server/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
from conda_store_server import conda_utils, utils
from pydantic import BaseModel, Field, ValidationError, constr, validator

ON_WIN = sys.platform.startswith("win")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already declared in another file. Can we move that definition somewhere where it makes the most sense and import everywhere? Ideally, this should be done in a way that minimizes potential import cycles.



def _datetime_factory(offset: datetime.timedelta):
"""utcnow datetime + timezone as string"""
Expand Down Expand Up @@ -194,20 +196,20 @@ class Settings(BaseModel):
metadata={"global": True},
)

default_uid: int = Field(
os.getuid(),
default_uid: int | None = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

This syntax is Python 3.10+ IIUC. Let's use Optional[int] here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this should be fine, but we need to add from __future__ import annotations. I guess we aren't testing old Python versions anywhere. What's the oldest version conda-store should support.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the minimal version is. The point I was trying to make: we don't use it anywhere else. I don't see why we should introduce this and immediately bump our lowest supported version to 3.10+ or require an extra import. Let's just use Optional, like everywhere else.

None if ON_WIN else os.getuid(),
description="default uid to assign to built environments",
metadata={"global": True},
)

default_gid: int = Field(
os.getgid(),
default_gid: int | None = Field(
None if ON_WIN else os.getgid(),
description="default gid to assign to built environments",
metadata={"global": True},
)

default_permissions: str = Field(
"775",
default_permissions: str | None = Field(
None if ON_WIN else "775",
description="default file permissions to assign to built environments",
metadata={"global": True},
)
Expand Down
11 changes: 8 additions & 3 deletions conda-store-server/conda_store_server/server/app.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import logging
import os
import posixpath
import sys

import conda_store_server
Expand Down Expand Up @@ -198,9 +199,9 @@ def trim_slash(url):
app = FastAPI(
title="conda-store",
version=__version__,
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"),
docs_url=posixpath.join(self.url_prefix, "docs"),
redoc_url=posixpath.join(self.url_prefix, "redoc"),
contact={
"name": "Quansight",
"url": "https://quansight.com",
Expand Down Expand Up @@ -335,6 +336,10 @@ def start(self):

# start worker if in standalone mode
if self.standalone:
address = "localhost" if self.address == "0.0.0.0" else self.address
print(
f"Starting standalone conda-store server at http://{address}:{self.port}"
)
Copy link
Contributor

@nkaretnikov nkaretnikov Oct 18, 2023

Choose a reason for hiding this comment

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

I don't like this. If we're using self.address, we should print self.address. I understand this is trying to be helpful, but it'll only make issues harder to debug later. I'd suggest to remove this entirely.

import multiprocessing

multiprocessing.set_start_method("spawn")
Expand Down
4 changes: 3 additions & 1 deletion conda-store-server/conda_store_server/server/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,9 @@ async def post_login_method(
samesite="strict",
domain=self.cookie_domain,
# set cookie to expire at same time as jwt
max_age=(authentication_token.exp - datetime.datetime.utcnow()).seconds,
max_age=int(
(authentication_token.exp - datetime.datetime.utcnow()).total_seconds()
),
nkaretnikov marked this conversation as resolved.
Show resolved Hide resolved
)
return response

Expand Down
3 changes: 2 additions & 1 deletion conda-store-server/conda_store_server/storage.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import io
import os
import posixpath
import shutil

import minio
Expand Down Expand Up @@ -223,7 +224,7 @@ def get(self, key):
return f.read()

def get_url(self, key):
return os.path.join(self.storage_url, key)
return posixpath.join(self.storage_url, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you remember what failed without this? The effect of this change will be converting Windows paths to posixpaths:

>>> import posixpath
>>>
>>> posixpath.join("foo", "bar")
'foo/bar'
>>>
>>> import os
>>> os.path.join("foo","bar")
'foo\\bar'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows backslash paths are not correct for file URLs. Without this change, some of the buttons in the UI don't work because they use %5C (URL encoded \) instead of /.


def delete(self, db, build_id, key):
filename = os.path.join(self.storage_path, key)
Expand Down
43 changes: 41 additions & 2 deletions conda-store-server/conda_store_server/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,52 @@ def chdir(directory: pathlib.Path):
os.chdir(current_directory)


def du(path):
"""
Pure Python equivalent of du -sb

Based on https://stackoverflow.com/a/55648984/161801
"""
if os.path.islink(path):
return os.lstat(path).st_size
if os.path.isfile(path):
st = os.lstat(path)
return st.st_size
apparent_total_bytes = 0
have = set()
for dirpath, dirnames, filenames in os.walk(path):
apparent_total_bytes += os.lstat(dirpath).st_size
for f in filenames:
fp = os.path.join(dirpath, f)
if os.path.islink(fp):
apparent_total_bytes += os.lstat(fp).st_size
continue
st = os.lstat(fp)
if st.st_ino in have:
continue
have.add(st.st_ino)
apparent_total_bytes += st.st_size
for d in dirnames:
dp = os.path.join(dirpath, d)
if os.path.islink(dp):
apparent_total_bytes += os.lstat(dp).st_size

return apparent_total_bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you seen an implementation I shared here? #559 (comment)

It fixes some style issues and the code is shorter. I also find it more readable because variables are shorter. The first two ifs can be written as one.

Direct link to gist: https://gist.github.com/nkaretnikov/1a66b90a74fa805f1022e90252e54c87

There are also tests there that you could re-use, like the file/directory creation logic. Ignore the part where I check against tools, it should just check against hardcoded numbers. But it's not a blocker anyway.



def disk_usage(path: pathlib.Path):
if sys.platform == "darwin":
cmd = ["du", "-sAB1", str(path)]
else:
elif sys.platform == "linux":
cmd = ["du", "-sb", str(path)]
else:
return str(du(path))

return subprocess.check_output(cmd, encoding="utf-8").split()[0]
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)
return output
nkaretnikov marked this conversation as resolved.
Show resolved Hide resolved


@contextlib.contextmanager
Expand Down
11 changes: 10 additions & 1 deletion conda-store-server/conda_store_server/worker/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,18 @@ def start(self):
argv = [
"worker",
"--loglevel=INFO",
"--beat",
]

# The default Celery pool requires this on Windows. See
# https://stackoverflow.com/questions/37255548/how-to-run-celery-on-windows
if sys.platform == "win32":
os.environ.setdefault("FORKED_BY_MULTIPROCESSING", "1")
else:
# --beat does not work on Windows
argv += [
"--beat",
]

if self.concurrency:
argv.append(f"--concurrency={self.concurrency}")

Expand Down
Loading
Loading