fix: ensure installed binaries have executable permissions#1233
fix: ensure installed binaries have executable permissions#1233ANAS727189 wants to merge 2 commits intofortran-lang:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where binaries installed via fpm install did not have executable permissions set, requiring users to manually run chmod +x. The fix adds a new set_executable function that uses the C chmod function for efficiency on Unix systems, with a fallback to shell commands during bootstrap, and skips the operation on Windows.
Changes:
- Added
set_executablesubroutine infpm_filesystem.F90that sets executable permissions (755) on Unix systems - Modified
install_executableininstaller.f90to callset_executableafter installation - Optimized exe_path calculation to avoid duplication for macOS rpath operations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/fpm_filesystem.F90 | Adds new set_executable subroutine with C binding to chmod for non-bootstrap builds and shell fallback for bootstrap |
| src/fpm/installer.f90 | Calls set_executable after installing executables and reuses exe_path variable for macOS rpath commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end if | ||
| #else | ||
| ! Fallback using shell command | ||
| call run("chmod +x " // filename, echo=.false., verbose=.false., exitstat=stat) |
There was a problem hiding this comment.
The bootstrap fallback path silently ignores chmod failures by not checking the exitstat value. While a warning is logged in the non-bootstrap path when chmod fails, the bootstrap path should also handle failures consistently. Consider checking the stat variable and logging a warning if the command fails.
| call run("chmod +x " // filename, echo=.false., verbose=.false., exitstat=stat) | |
| call run("chmod +x " // filename, echo=.false., verbose=.false., exitstat=stat) | |
| if (stat /= 0) then | |
| write(stderr, *) "Warning: Failed to set executable permissions on: ", filename | |
| end if |
| subroutine set_executable(filename) | ||
| character(len=*), intent(in) :: filename | ||
| integer :: stat | ||
|
|
||
| if (.not. os_is_unix()) return | ||
|
|
||
| #ifndef FPM_BOOTSTRAP | ||
| ! Use C library chmod (faster, standard) | ||
| ! o'755' is octal for rwxr-xr-x | ||
| stat = c_chmod(filename // c_null_char, int(o'755', c_int)) | ||
| if (stat /= 0) then | ||
| write(stderr, *) "Warning: Failed to set executable permissions on: ", filename | ||
| end if | ||
| #else | ||
| ! Fallback using shell command | ||
| call run("chmod +x " // filename, echo=.false., verbose=.false., exitstat=stat) | ||
| #endif | ||
|
|
||
| end subroutine set_executable |
There was a problem hiding this comment.
The new set_executable function lacks test coverage. Since the repository has a test_filesystem.f90 file with tests for other filesystem functions, consider adding a test to verify that executable permissions are correctly set on Unix systems and that the function is a no-op on Windows.
src/fpm/installer.f90
Outdated
| add_rpath: if (self%os==OS_MACOS) then | ||
|
|
||
| exe_path = join_path(self%install_destination(self%bindir) , basename(executable)) | ||
| ! exe_path is already calculated above |
There was a problem hiding this comment.
The comment is misleading because it suggests the removed line was incorrect or unnecessary, when in reality it was moved earlier for efficiency. Consider clarifying this as "Reused from above" or simply removing the comment since the code is self-explanatory.
Fixes #314
This PR fixes an issue where binaries installed via
fpm install(or./install.sh) did not have the executable bit set (resulting inrw-r--r--), requiring users to manually runchmod +x.Changes Implemented
src/fpm_filesystem.F90: Added aset_executablesubroutine.c_chmod(viaiso_c_binding) for standard builds (efficient and standard).chmod +xfor the bootstrap phase (#ifdef FPM_BOOTSTRAP).os_is_unix()check to ensure it only runs on relevant platforms (no-op on Windows).Testing
Created a new project (
fpm new test_perms), installed it, and verified vials -lthat the resulting binary in~/.local/bin(or prefix) has-rwxr-xr-xpermissions.fpm test. All 35 tests PASSED.