Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
99 changes: 73 additions & 26 deletions .git-hooks/pre-push.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,31 +41,37 @@
import subprocess
import sys
import tempfile
from pathlib import Path

# Git invokes this hook with the ambient environment, which does not
# necessarily have PYTHONPATH configured (e.g., on Windows). Locate
# "dev_tools/pylibs" relative to this script so "edgutil" can be imported.
sys.path.insert(
0, str(Path(__file__).resolve().parents[1] / 'dev_tools' / 'pylibs')
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't love this, I'd prefer people actual configure the environment (which dev-init.py should help with ... which also installs the script?). However, if we're only doing this for the pre-push.py script (as a sort of "balancing act" to handle weird edge cases), I think that's fine.

@soroshsabz soroshsabz Oct 16, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Understood: it is deliberately only in pre-push.py, as the balancing act you describe. Every other tool is invoked from a configured environment, but git runs this hook with the ambient environment, and nothing surfaced that distinction to the user today (the failure was an ImportError inside a closing console window). And yes, dev-init.py installs the hook (it writes the .git/hooks/pre-push shim that executes .git-hooks/pre-push.py), so dev-init users get the self-locating version automatically.


import edgchanges
import edgutil

from pathlib import Path

if platform.system() == 'Windows':
# On Windows we need to use the kernel API to detect if this script is
# running in a standalone console window (if so, there will be only one
# processes at this point).
import ctypes

kernel32 = ctypes.WinDLL('kernel32', use_last_error = True)
process_array = (ctypes.c_uint * 2)()
num_processes = kernel32.GetConsoleProcessList(process_array, 2)
if num_processes != 1:
# When not running in a standalone console window, the script relaunches
# itself to make sure the user has an interactive shell. Then it forwards
# the exit code from the delegate process.
# The standard input of this hook carries the push ref list supplied by
# git, so it cannot be used to prompt the user. When a console is
# attached, open the console input device directly ("CONIN$" is the
# Windows counterpart of "/dev/tty") so that all prompts and output stay
# in the user's current terminal.
try:
sys.stdin = open('CONIN$', 'r')
except OSError:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Confession: I wrote the original logic here, I'm not a "Windows guy" but it seemed to work and be reasonably correct; I'll defer to others that know Windows better as to if this is an improvement.

@soroshsabz soroshsabz Oct 16, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the context! The Windows rationale: CONIN$ is the console input device and works like /dev/tty: it can be opened regardless of what stdin is connected to (git feeds the hook the ref list on stdin, so stdin itself can't serve the prompts). With a console attached (cmd, PowerShell, Windows Terminal), everything now stays in the user's current terminal. Your relaunch is preserved as the fallback for the cases where CONIN$ cannot open (GUI git clients, mintty-style pseudo-terminals), so that intent is kept; that fallback window also now waits for Enter on failure so its output can't flash away.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

cmd and PowerShell are shells, and Windows Terminal is a terminal GUI. From the Windows perspective, none of them are consoles; only conhost.exe and openconsole.exe are.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For performance, NtQueryVolumeInformationFile should be used, but since this is a Python script, the current approach is acceptable.

@wchilders-nvidia wchilders-nvidia Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@YexuanXiao when you say "the current approach" you mean the CONIN approach, correct? (or was this advocating for the kernel32 approach in main?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the reference! Agreed this isn't performance-sensitive here (it runs once per push), so the single GetConsoleMode call through ctypes is fine. Noted for anyone porting the check to C++ later, where the NtQueryVolumeInformationFile approach would matter.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right, thanks for the correction: cmd and PowerShell are shells, and Windows Terminal is a terminal emulator; the console itself is hosted by conhost.exe (or OpenConsole.exe, including the ConPTY sessions used by Windows Terminal). The relevant condition for this code is simply whether the process is attached to such a console session, which is the case for a push from cmd/PowerShell/Windows Terminal, and not for GUI git clients or mintty-style pseudo terminals (where CONIN$ cannot be opened and the relaunch fallback kicks in). My wording above was sloppy; no behavior change is needed.

# No console is attached (e.g., a GUI git client or a pseudo terminal
# such as mintty). Relaunch in a standalone console window so that the
# user can still respond to interactive prompts.
delegate_proc = subprocess.run(
[
sys.executable,
__file__
],
creationflags = subprocess.CREATE_NEW_CONSOLE
creationflags = subprocess.CREATE_NEW_CONSOLE,
env = {**os.environ, 'EDG_PREPUSH_DELEGATE': '1'}
)
sys.exit(delegate_proc.returncode)
else:
Expand Down Expand Up @@ -138,6 +144,7 @@ class PrePushChecker:

def __init__(self):
self.errors = 0
self.abort = False

def confirm_okay(self):
'''Ask the user whether a policy warning is acceptable.
Expand All @@ -160,7 +167,8 @@ def confirm_okay(self):
return
if response == 'q':
self.errors = 1
sys.exit(1)
self.abort = True
return

print(
"Please answer 'y' (to accept), 'n' (to continue check) or "
Expand All @@ -169,7 +177,9 @@ def confirm_okay(self):
tries += 1
if tries >= 10:
print('Too many attempts.')
sys.exit(1)
self.errors = 1
self.abort = True
return

def reject(self):
'''Record a problem that can't be accepted, without asking.'''
Expand All @@ -181,8 +191,9 @@ def check_commits(self):

As commits are walked, check to make sure they match formatting guidance.

Returns the oldest new commit hash, or None if there are no new commits.
Exits immediately if a commit subject fails the PR-number policy.
Returns the oldest new commit hash, or None if there are no new commits
(or if a commit subject fails the PR-number policy, in which case an
error is recorded and the check sequence is aborted).
'''
first_new_commit = None

Expand All @@ -206,7 +217,9 @@ def check_commits(self):
print(
" [POLICY] Commit message doesn't contain valid PR number (or [])."
)
sys.exit(1)
self.errors = 1
self.abort = True
return None

first_new_commit = commit

Expand Down Expand Up @@ -285,12 +298,18 @@ def check_changed_files(self, first_new_commit):
)

if file_change_check_proc.returncode != 0:
sys.exit(1)
# Record the failure and return (rather than exiting) so that the
# status propagates through checker.run() and main(), allowing the
# delegate window pause to run.
self.errors = 1
return

with open(output_file, 'r') as file_handle:
results = json.load(file_handle)

for file_path_str, file_results in results.items():
if self.abort:
break
Comment thread
coderabbitai[bot] marked this conversation as resolved.
print(f" Checking file: {file_path_str}")

# A pending Changes entry with any of these problems fails the
Expand All @@ -309,6 +328,8 @@ def check_changed_files(self, first_new_commit):
for error in format_errors:
print(f" - {edgchanges.describe_error(file_path_str, error)}")
confirm_okay()
if self.abort:
break

misspellings = file_results['misspellings']
if misspellings is None:
Expand All @@ -327,13 +348,17 @@ def check_changed_files(self, first_new_commit):
)
print_indented('\n'.join(hint_lines), ' ')
confirm_okay()
if self.abort:
break

typos = file_results['typos']
if len(typos) != 0:
print(' New typos:')
for typo in typos:
print(f" - {typo}")
confirm_okay()
if self.abort:
break

coding_errors = file_results['coding_errors']
if len(coding_errors) != 0:
Expand All @@ -342,36 +367,58 @@ def check_changed_files(self, first_new_commit):
print(f" Missing comment on #endif:")
print_indented('\n'.join(endif_hits), ' ')
self.confirm_okay()
if self.abort:
break

else_hits = coding_errors['missing_elses']
if len(else_hits) != 0:
print(f" Missing comment on #else:")
print_indented('\n'.join(else_hits), ' ')
self.confirm_okay()
if self.abort:
break

closing_comments = coding_errors['closing_comments']
if len(closing_comments) != 0:
print_indented('\n'.join(closing_comments), ' ')
self.confirm_okay()
if self.abort:
break

overlong_lines = file_results['overlong_lines']
if len(overlong_lines) != 0:
for line in edgutil.format_overlong_lines(overlong_lines, ' '):
print(line)
confirm_okay()
if self.abort:
break

def run(self) -> bool:
'''Execute the full pre-push check sequence.'''
def run(self) -> int:
'''Execute the full pre-push check sequence. Return the exit status.'''
first_new_commit = self.check_commits()

# Skip per file checks if we didn't actually detect a new commit.
if first_new_commit is not None:
if not self.abort and first_new_commit is not None:
self.check_changed_files(first_new_commit)

return self.errors == 0
return 0 if (self.errors == 0 and not self.abort) else 1

def pause_delegate_window() -> None:
'''When running as a relaunch delegate, wait for the user before the
standalone console window closes so that failure output can be read.'''
if os.environ.get('EDG_PREPUSH_DELEGATE') != '1':
return

try:
input('Push checks failed; press Enter to close this window. ')
except EOFError:
pass

def main():
checker = PrePushChecker()
sys.exit(0 if checker.run() else 1)
returncode = checker.run()
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if returncode != 0:
pause_delegate_window()
sys.exit(returncode)

main()
24 changes: 24 additions & 0 deletions HACKING.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,30 @@ If direnv is not an option, manually add `<EDG PROJECT DIR>/dev_tools/bin` (or
variables respectively. This will make the EDG development tools and their
supporting Python libraries available.

For Windows users, a PowerShell counterpart of the direnv setup is provided
at the root of the repository; from a PowerShell prompt dot source it to
configure the current session:

```
. .\dev-env.ps1
```

(If PowerShell script execution is disabled, first run
`Set-ExecutionPolicy -Scope CurrentUser RemoteSigned`, or use the manual
environment variable configuration described above.)

> [!WARNING]
>
> Windows users should ensure their clone uses LF line endings, e.g., by
> setting `git config --global core.autocrlf input` before cloning (an
> existing clone can be repaired by setting that configuration for the
> repository -- `git config core.autocrlf input`, run inside it -- and
> re-checking out the tree). The default Windows Git configuration
> (`core.autocrlf true`) produces a CRLF checkout, which breaks both the
> build (e.g., `mk_errinfo` cannot parse `src/error_msg.txt`) and the
> Docker based workflow (scripts mounted into the container fail on CRLF
> shebangs).

## Docker Based (Recommended)

To get started with Docker, it is recommended that Windows and MacOS users
Expand Down
50 changes: 50 additions & 0 deletions dev-env.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM
# Exceptions.
# See https://edgcpp.org/LICENSE.txt for license information.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

# Windows counterpart of the ".envrc" file used with direnv on POSIX systems:
# from a PowerShell prompt, dot source this script to make the EDG
# development tools and their supporting Python libraries available in the
# current session:
#
# . .\dev-env.ps1
#
# This prepends "dev_tools/win-bin" and "dev_tools/bin" to the PATH and
# "dev_tools/pylibs" to the PYTHONPATH. The script may be dot sourced from
# any directory and repeated dot sourcing is harmless.

$edgRepoRoot = $PSScriptRoot

$edgToolDirs = @(
(Join-Path $edgRepoRoot 'dev_tools\win-bin'),
(Join-Path $edgRepoRoot 'dev_tools\bin')
)

# Prepend the tool directories if either is missing (a duplicate entry is
# harmless) so that "dev_tools\win-bin" stays ahead of "dev_tools\bin" in
# the search order and the ".bat" shims keep precedence over the
# extensionless scripts.
$edgNeedPrepend = $false
foreach ($edgToolDir in $edgToolDirs) {
$edgOnPath = @($env:Path -split ';' | Where-Object { $_ -ieq $edgToolDir })
if ($edgOnPath.Count -eq 0) {
$edgNeedPrepend = $true
}
}
if ($edgNeedPrepend) {
$env:Path = "$($edgToolDirs -join ';');$env:Path"
}

$edgPyLibs = Join-Path $edgRepoRoot 'dev_tools\pylibs'
if (-not (@($env:PYTHONPATH -split ';' |
Where-Object { $_ -ieq $edgPyLibs }))) {
if ($env:PYTHONPATH) {
$env:PYTHONPATH = "$edgPyLibs;$env:PYTHONPATH"
} else {
$env:PYTHONPATH = $edgPyLibs
}
}

Remove-Variable -Name edgRepoRoot, edgToolDirs, edgToolDir, edgOnPath, `
edgNeedPrepend, edgPyLibs -ErrorAction SilentlyContinue
40 changes: 38 additions & 2 deletions dev-init.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,25 @@ async def check_edg_python() -> InitResult:

if platform.system() == 'Windows':
find_edgy.add_failure_commentary(
'Open the Windows environment variable settings:'
'For the current PowerShell session only, run (from the repository '
'root):'
)
find_edgy.add_failure_commentary(
' . .\\dev-env.ps1',
allow_wrap = False
)
find_edgy.add_failure_commentary(
'and re-run this script; or, to configure new terminals '
'permanently, run:'
)
find_edgy.add_failure_commentary(
' [Environment]::SetEnvironmentVariable("Path", '
'[Environment]::GetEnvironmentVariable("Path", "User") '
f"+ ';{script_path}', 'User')",
allow_wrap = False
)
find_edgy.add_failure_commentary(
'Alternatively, open the Windows environment variable settings:'
)
find_edgy.add_failure_commentary(
' "C:\\Windows\\system32\\rundll32.exe" '
Expand Down Expand Up @@ -271,7 +289,25 @@ async def check_edg_python() -> InitResult:

if platform.system() == 'Windows':
find_edg_tools.add_failure_commentary(
'Open the Windows environment variable settings:'
'For the current PowerShell session only, run (from the repository '
'root):'
)
find_edg_tools.add_failure_commentary(
' . .\\dev-env.ps1',
allow_wrap = False
)
find_edg_tools.add_failure_commentary(
'and re-run this script; or, to configure new terminals '
'permanently, run (appending to any existing user PYTHONPATH):'
)
find_edg_tools.add_failure_commentary(
' [Environment]::SetEnvironmentVariable("PYTHONPATH", '
'((@([Environment]::GetEnvironmentVariable("PYTHONPATH", "User"), '
f"'{pylibs_path}') | Where-Object {{ $_ }}) -join ';'), 'User')",
allow_wrap = False
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
find_edg_tools.add_failure_commentary(
'Alternatively, open the Windows environment variable settings:'
)
find_edg_tools.add_failure_commentary(
' "C:\\Windows\\system32\\rundll32.exe" '
Expand Down
18 changes: 15 additions & 3 deletions dev_tools/pylibs/edgdocker/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,17 @@ def _check_parents(mapped_dirs: List[Path], workspace_dir: Path) -> None:
This can be valid (from a Docker perspective), but it's an advanced use case
and can be a dangerous mistake (so we don't permit it).
'''
# Resolve the workspace so that a relative path cannot bypass the drive
# comparison below (and "normcase" so that Windows drive letters that
# differ only in case compare equal).
workspace_dir = workspace_dir.resolve()
workspace_drive = os.path.normcase(os.path.splitdrive(str(workspace_dir))[0])
for mapped_dir in mapped_dirs:
if os.path.normcase(os.path.splitdrive(str(mapped_dir))[0]) != workspace_drive:
# The two paths are on different drives (which can only happen on
# Windows), so neither path can contain the other and the check below
# does not apply.
continue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We probably want to do a bit more interrogation as this is a sort of safety check(?)

@soroshsabz soroshsabz Oct 16, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good point. Replaced the blanket except ValueError with an explicit check in 3d96ddb: the overlap check is now skipped only when the two paths are on genuinely different drives (os.path.splitdrive comparison), documented as "neither path can contain the other". Same-drive paths always get the full commonpath check, and any other unexpected condition now raises loudly instead of being silently skipped.

common_dir = Path(os.path.commonpath([mapped_dir, workspace_dir]))
if common_dir == workspace_dir:
print('Current workspace is the parent of a mapped dir:')
Expand Down Expand Up @@ -400,7 +410,7 @@ def exec_docker_cmd(image_name: str, workspace_dir: Path,
f"EDG_USER_NAME={render_user_name()}"
]

if os.isatty(sys.stdin.fileno()):
if edgutil.is_interactive_terminal():
# Make the container interactive.
docker_args.append('-it')
else:
Expand Down Expand Up @@ -483,13 +493,15 @@ def exec_docker_cmd(image_name: str, workspace_dir: Path,

if platform.system() == 'Windows':
# Windows does not properly handle the execvp. Instead, keep this process
# around, and simply forward on stdin, stdout, and stderr.
subprocess.run(
# around, and simply forward on stdin, stdout, and stderr. Forward the
# exit status of the container so that callers observe failures.
completed_proc = subprocess.run(
docker_args,
stdin = sys.stdin,
stdout = sys.stdout,
stderr = sys.stderr
)
sys.exit(completed_proc.returncode)
else:
# Execute the docker command, replacing the current process.
os.execvp('docker', docker_args)
Expand Down
Loading