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 3 commits 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
3 commits
Select commit Hold shift + click to select a range
acd0876
DAOS-18933 ci: fix PermissionError in pre-commit clang-format hook
kanard38 May 6, 2026
ff92294
Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…
kanard38 May 11, 2026
00648b4
Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…
kanard38 May 12, 2026
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.

Copy link
Copy Markdown
Contributor

@janekmi janekmi May 13, 2026

Choose a reason for hiding this comment

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

Fake scons environment shutting up pylint on SCons files

I guess it is no longer true since it is being used outside of this context. You have to consider all the scripts as they are being used. Fixing weakness of the shim in the code is a bad approach.

We either will find a way to load a different shim so you will again have:

  1. shim dedicated only to pylint runs which can be as small as you want and
  2. another shim(?) which is actually usable in the context it is being used.

OR the shim we use stops being pylint-only and we have to make it actually usable.

Copy link
Copy Markdown
Contributor Author

@knard38 knard38 May 13, 2026

Choose a reason for hiding this comment

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

I agree with you that the proposed solution is not ideal from an architectural standpoint.

From my understanding, the shim ends up in the pre-commit context not by design, but accidentally — utils/sl/fake_scons is added to PYTHONPATH by the standard DAOS dev environment (needed for daos_pylint.py), so any standalone Python invocation, including the pre-commit hook, picks it up. I do not think it was intended to be used there.
At this point, the shim still has no real functionality (mostly no-ops), with just a few exceptions needed by pylint.

As I see it, each solution has its own trade-offs:

  • Fix in extra.py (current PR):
    • Pro: minimal and targeted — defends the one real caller without touching the shim, no risk of side effects.
    • Con: works around a shim weakness rather than addressing it at the source.
  • Two shims: keep a no-op shim for pylint, add a functional one for other contexts.
    • Pro: clean separation of concerns.
    • Con: requires a loading mechanism to select the right shim per context.
  • Make the shim functional: implement WhereIs (and similar) properly in the existing shim.
    • Pro: fixes the root cause once for all callers.
    • Con: changes the shim's pylint-only contract and could have unintended side effects.

I do not have enough context on the build system to judge which approach fits best here.
@jolivier23 and @daltonbohning do you have any opinion on whether to change the shim or keep the fix in extra.py?

Copy link
Copy Markdown
Contributor

@daltonbohning daltonbohning May 13, 2026

Choose a reason for hiding this comment

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

utils/sl/fake_scons is added to PYTHONPATH by the standard DAOS dev environment

What is setting this? Personally, I've never run into this issue with the stub so I'm not sure what exactly the issue is

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