Skip to content

DAOS-18933 ci: fix PermissionError in pre-commit clang-format hook #18183

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.

Sign up for GitHub

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

Open
knard38 wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −3
Open

DAOS-18933 ci: fix PermissionError in pre-commit clang-format hook #18183

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions site_scons/site_tools/extra/extra.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#!/usr/bin/env python3
"""
(C) Copyright 2018-2022 Intel Corporation.
(C) Copyright 2025 Hewlett Packard Enterprise Development LP
(C) Copyright 2025-2026 Hewlett Packard Enterprise Development LP

SPDX-License-Identifier: BSD-2-Clause-Patent

Expand All @@ -12,6 +12,7 @@
"""
import os
import re
import shutil
import subprocess # nosec
import sys

Expand All @@ -30,8 +31,8 @@ def errprint(*args, **kwargs):


def _get_version_string():
clang_exe = WhereIs('clang-format')
if clang_exe is None:
clang_exe = WhereIs('clang-format') or shutil.which('clang-format')
Copy link
Copy Markdown
Contributor

@janekmi janekmi May 6, 2026

Choose a reason for hiding this comment

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

Why you decided to use both? Why not just replace the unreliable one with a reliable one?

Copy link
Copy Markdown
Contributor Author

@knard38 knard38 May 6, 2026

Choose a reason for hiding this comment

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

SCons.Script ships a no-op stub for WhereIs that is active whenever the module is imported outside a full SCons build (i.e. the pre-commit hook context).

On my machine:

>>> from SCons.Script import WhereIs; WhereIs('clang-format')
''                          # stub always returns ''

>>> import shutil; shutil.which('clang-format')
'/usr/bin/clang-format'     # stdlib finds it correctly

Replacing WhereIs entirely would fix the crash but lose the SCons-environment-aware lookup during actual builds. The or keeps WhereIs when it works and falls back to shutil.which when the stub is in effect.

Copy link
Copy Markdown
Contributor

@janekmi janekmi May 6, 2026

Choose a reason for hiding this comment

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

It is a little messy. 😅

Another thought. Why not use shutil.which('clang-format') in the WhereIs stub?

Copy link
Copy Markdown
Contributor Author

@knard38 knard38 May 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It is a little messy. 😅

Another thought. Why not use shutil.which('clang-format') in the WhereIs stub?

Did not wanted to touch to the fake_scons stubs as I am not sure of their purpose.
I am going to investigate as I agree that it should be a better solution: I have to check that I am not breaking anything (test, etc.).

Copy link
Copy Markdown
Contributor Author

@knard38 knard38 May 7, 2026

Choose a reason for hiding this comment

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

After investigating, I'd keep the fix in extra.py rather than changing the stub.

fake_scons is a pylint shim, as indicated by its docstring "Fake scons environment shutting up pylint on SCons files" — its sole purpose is to provide importable SCons.* stubs so that pylint can analyse SCons-based Python files without crashing on ImportError. Its functions deliberately do nothing — that is the intended contract.

The root cause of the crash is that utils/sl/fake_scons ends up on PYTHONPATH for any developer who sources the standard DAOS dev environment (it is needed for daos_pylint.py). As a result, any standalone Python invocation — including the pre-commit hook — resolves SCons.Script to the shim instead of the real SCons, and gets the no-op WhereIs stub.

If I am correct, making the stub functional with shutil.which would silently change its behaviour for pylint runs and could mask similar issues in the future. The fix in extra.py is the right level: it defends the one real caller against whatever WhereIs returns, regardless of which SCons.Script is on the path.

if not clang_exe:
return None
try:
rawbytes = subprocess.check_output([clang_exe, "-version"])
Expand Down
Loading