Add os.access function for file access checking#7274
Conversation
Summary of ChangesHello @luadebug, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new os.access function for checking file access permissions and refactors os.isexec to use it. This is a good improvement for checking execute permissions, especially on Unix-like systems. However, the change to os.isexec introduces a regression on Windows by removing the logic that checks for executable file extensions. My review includes a suggestion to fix this regression while retaining the benefits of the new os.access function.
…d add PE header checks
…mline PE header validation for Windows executables
xmake/core/base/os.lua
Outdated
| return true | ||
| if os.host() == "windows" then | ||
| local ext = path.extension(filepath):lower() | ||
| if ext == ".exe" or ext == ".cmd" or ext == ".bat" or ext == ".ps1" then |
There was a problem hiding this comment.
I thought about it again, and on Windows, simply checking if the file exists should be sufficient.
Using only the file extension isn't accurate; it might overlook some executable files, such as .sh scripts on msys2, and shell scripts might not even have a file extension.
There was a problem hiding this comment.
The current rules for filtering file extensions may break some things when built on msys2.
e.g. 7z.sh on msys2
xmake/core/base/os.lua
Outdated
| if os.host() == "windows" then | ||
| local ext = path.extension(filepath):lower() | ||
| if ext == ".exe" or ext == ".cmd" or ext == ".bat" or ext == ".ps1" then | ||
| if ext == ".exe" or ext == ".cmd" or ext == ".bat" or ext == ".ps1" or ext == ".sh" then |
There was a problem hiding this comment.
Simply adding .sh is not enough; many files without extensions are shell script files.
Therefore, we can only use os.isfile
There was a problem hiding this comment.
Therefore, we can only use os.isfile
If we could sort of use PIPE to capture output of streams would that help to filter out files without extensions shell script files? Such as core.base.pipe.lua thing. As PE file check would not work over shell scripts but might work over binary files without extension or with wrong extension, as for Windows OS example with PE file check we can figure out arch of executable and minimum Windows OS support (Windows XP) for example.
The current rules for filtering file extensions may break some things when built on msys2. e.g. 7z.sh on msys2
That is probably same issue with shell script files without extensions I guess...
There was a problem hiding this comment.
try this
function os.isexec(filepath)
assert(filepath)
if os.host() == "windows" then
local exts = {".exe", ".cmd", ".bat", ".ps1", ".sh"}
if os.isfile(filepath) then
-- detect file extension first
local extension = path.extension(filepath):lower()
if extension then
if table.contains(extension, exts) then
return true
end
end
-- detect file header
if winos.is_pefile(filepath) then
return true
end
-- detect shebang scripts
local file = io.open(filepath, "rb")
if file then
local header = file:read(2)
file:close()
if header == "#!" then
return true
end
end
end
for _, suffix in ipairs(exts) do
if os.isfile(filepath .. suffix) then
return true
end
end
elseif os.isfile(filepath) then
return os._access(filepath, "x")
end
return false
endThen we can improve the previous parsepe native interface and change it to the winos.is_pefile interface implementation.
like this
def is_pe_file(file_path):
try:
with open(file_path, 'rb') as f:
# Step 1: Check the DOS header "MZ"
mz_signature = f.read(2)
if mz_signature != b'MZ':
return False
# Step 2: Seek to the PE header
f.seek(60) # DOS header e_lfanew offset location
pe_offset = int.from_bytes(f.read(4), 'little')
# Step 3: Check the PE signature "PE\0\0"
f.seek(pe_offset)
pe_signature = f.read(4)
if pe_signature == b'PE\0\0':
return True
except Exception as e:
print(f"Error reading file: {e}")
return False
# Example usage
print(is_pe_file("example.exe"))|
I need to refactor binutils and add the binutils.format interface to replace is_pefile. please wait #7278 |
It looks like
os.isexeccould be improved?