-
Notifications
You must be signed in to change notification settings - Fork 934
Fix/windows support #338
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
Fix/windows support #338
Conversation
For point 2: As a general philosophy, I think it is best to avoid setting any system / global git properties -- instead prompting the user to do so or emitting a warning. I think just checking for the option and printing warn logs is a better way to handle this problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me, would have to run some tests but I don't have easy access to a windows machine right now
hmm im not sure why the test fails and I cant test on my windows PC atm, can you take a look @BareninVitalya ? |
CI fails because I will update the function to print a warning instead of raising an exception if checking or setting |
Fix: Gracefully handle Windows long path support checkChecking whether long path support is enabled on Windows turned out to be more complex due to Git’s configuration priority levels ( For Git to correctly support long file paths on Windows, the setting To avoid this, we now detect whether the current process is running with Administrator privileges before attempting to check the |
Wait, I'm not sure I get it Why do you check using Imo just checking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't think warning on every non-elevated execution is desirable)
To clarify: when you run You can verify this with a simple test: git config --system core.longpaths true
git config core.longpaths
# → still returns false (because this checks --global, not --system) This behavior was reproducible on my local machine under both a standard and an admin user. Unless I agree that issuing a warning every time the app runs without elevation is not ideal. I need to think about how to approach this problem differently. |
Here's what I propose as a balanced solution: I keep the current admin check and the conditional system-level if sys.platform == "win32":
try:
if ctypes.windll.shell32.IsUserAnAdmin():
stdout, _ = await run_command("git", "config", "--system", "core.longpaths")
if stdout.decode().strip().lower() != "true":
print(
f"{Colors.BROWN}WARN{Colors.END}: {Colors.RED}Git clone may fail on Windows "
f"due to long file paths:{Colors.END}",
)
print(f"{Colors.RED}To avoid this issue, consider enabling long path support with:{Colors.END}")
print(f"{Colors.RED} git config --system core.longpaths true{Colors.END}")
print(f"{Colors.RED}Note: This command may require administrator privileges.{Colors.END}")
except RuntimeError:
# Ignore if checking 'core.longpaths' fails due to lack of administrator rights.
pass This way:
AlternativeI also considered catching the actual Git failure (e.g., However, this has drawbacks:
Let me know if this middle-ground solution makes sense to you. |
Are you sure? I was checking the documentation |
I conducted extensive testing on two separate Windows 10 machines (different builds), using Git versions 2.46 and 2.50. Here's what I observed:
Now check: stdout, _ = await run_command("git", "config", "--global", "core.longpaths") This is enough if users have not configured |
Maybe I am missing something but why is |
You are right, for our case this is indeed sufficient and it behaves as expected without the need to explicitly check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine by me but I think one of your commits is unsigned and prevents merge
can you try to fix that?
57fe67b
to
951171e
Compare
I tried to sign my commit using interactive rebase. As a result, other commits in the branch were rewritten and lost their original signatures. Can I create a new PR with only my signed change to fix the issue? |
You could try rebasing again and only signing the 2 commits that are missing a signature I think, but yeah this is not very convenient Also, instead of making a PR with only your signed changes you could just force push on this one Let me know if you need help I'll see what I can do to make it less of a pain in the ass |
13f00ed
to
3e4e661
Compare
Co-authored-by: Nicolas Iragne <nicoragne@hotmail.fr>
…eanup (coderamp-labs#349) * refactor: centralize PAT validation, streamline repo checks & housekeeping * `.venv*` to `.gitignore` * `# type: ignore[attr-defined]` hints in `compat_typing.py` for IDE-agnostic imports * Helpful PAT string in `InvalidGitHubTokenError` for easier debugging * Bump **ruff-pre-commit** hook → `v0.12.1` * CONTRIBUTING: * Require **Python 3.9+** * Recommend signed (`-S`) commits * PAT validation now happens **only** in entry points (`utils.auth.resolve_token` for CLI/lib, `server.process_query` for Web UI) * Unified `_check_github_repo_exists` into `check_repo_exists`, replacing `curl -I` with `curl --silent --location --write-out %{http_code} -o /dev/null` * Broaden `_GITHUB_PAT_PATTERN` * `create_git_auth_header` raises `ValueError` when hostname is missing * Tests updated to expect raw HTTP-code output * Superfluous “token can be set via `GITHUB_TOKEN`” notes in docstrings * `.gitingestignore` & `.terraform` from `DEFAULT_IGNORE_PATTERNS` * Token validation inside `create_git_command` * Obsolete `test_create_git_command_invalid_token` * Adjust `test_clone.py` and `test_git_utils.py` for new status-code handling * Consolidate mocks after token-validation relocation BREAKING CHANGE: `create_git_command` no longer validates GitHub tokens; callers must ensure tokens are valid (via `validate_github_token`) before invoking lower-level git helpers. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…oderamp-labs#352) Frontend * introduce Tailwind CSS (package.json, tailwind.config.js, input CSS) * build site.css on-the-fly (removed tracked artefact; added .gitignore) * new favicon/icon assets & template cleanup * split JS into modular files Docker * replace single-stage image with 3-stage build • css-builder (Node 20 alpine) → compiles Tailwind • python-builder installs project with PEP 621 metadata • runtime image copies site-packages + compiled CSS, runs as uid 1000 CI/CD * ci.yml: cache by pyproject.toml, install with `pip -e .[dev]` * new frontend job builds/archives CSS after tests * publish.yml: build CSS first, then wheel/sdist; trusted OIDC upload * tidy scorecard workflow Core library * clone.py, parser & utils now resolve tags in addition to branches/commits * fallback branch/tag discovery when `git ls-remote` fails * compat\_func.py back-ports Path.readlink / str.removesuffix for Py 3.8 Tooling & docs * add `[dev]` extra, drop requirements-dev.txt & its pre-commit fixer * refreshed CONTRIBUTING.md with Node/Tailwind instructions * updated tests for new tag logic
3e4e661
to
e10bfc0
Compare
Hmm feel free to add me on discord (jackylafrite) we can try and fix that history |
Thank a lot. I think I was able to solve the problems. |
Fix: Improve Windows Compatibility
This PR addresses two key issues encountered when running the project on Windows:
1. Avoid
--reload
withuvicorn
on WindowsWhen launching FastAPI with
uvicorn --reload
, Windows uses_WindowsSelectorEventLoop
, which does not supportasyncio.create_subprocess_exec
.This breaks functionality such as
check_repo_exists
, which depends on subprocess support.Proposal:
Add the following note to the documentation (e.g.,
README.md
or developer guide):2. Automatically enable Git long path support
Git on Windows may fail when cloning repositories with deep paths:
Fix:
The
ensure_git_installed()
function was updated.If the platform is Windows, the function now automatically enables long path support: