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.).

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