Skip to content

Fix LD_LIBRARY_PATH handling for shared libraries during fpm test #1259

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
ZainabTravadi wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix LD_LIBRARY_PATH handling for shared libraries during fpm test #1259

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
724fc91
Fix LD_LIBRARY_PATH handling for shared libraries during fpm test
ZainabTravadi Mar 12, 2026
f79600e
Restrict library path detection to shared libraries and improve reada…
ZainabTravadi Mar 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
11 changes: 9 additions & 2 deletions src/fpm_targets.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1377,18 +1377,25 @@ subroutine get_library_dirs(model, targets, shared_lib_dirs)

integer :: i
type(string_t) :: temp


! Start with an empty list
allocate(shared_lib_dirs(0))

do i = 1, size(targets)
associate(target => targets(i)%ptr)
! Only consider shared and archive library targets
if (all(target%target_type /= [FPM_TARGET_SHARED,FPM_TARGET_ARCHIVE])) cycle
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The PR description focuses on fixing runtime discovery for shared libraries, but this change also includes FPM_TARGET_ARCHIVE. Static archives don’t affect LD_LIBRARY_PATH, so either (a) restrict the condition to FPM_TARGET_SHARED only, or (b) update the PR description to explicitly state why archive targets should be included here.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

Using all(target%target_type /= [ ... ]) on a scalar-to-array comparison is correct but non-obvious to many readers. Consider rewriting this as a clearer membership test (e.g., negate an any(target%target_type == [...])) to reduce the chance of misinterpretation during future maintenance.

Suggested change
if (all(target%target_type /= [FPM_TARGET_SHARED,FPM_TARGET_ARCHIVE])) cycle
if (.not. any(target%target_type == [FPM_TARGET_SHARED,FPM_TARGET_ARCHIVE])) cycle

Copilot uses AI. Check for mistakes.
! Always include the output_dir for shared libraries
! Avoid duplicates
Comment on lines +1389 to +1390
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

These comments are misleading with the current logic: the code path also applies to FPM_TARGET_ARCHIVE due to the filter above. Please adjust the wording to match the implementation (or adjust the implementation to match the comment).

Copilot uses AI. Check for mistakes.
if (target%output_dir .in. shared_lib_dirs) cycle
temp = string_t(target%output_dir)
call add_strings(shared_lib_dirs, temp)
end associate
end do


! This fix ensures that the directory where shared libraries are produced
! (target%output_dir) is always included in the list returned, so LD_LIBRARY_PATH
! will contain the correct path for test execution. No interface changes, no filesystem scanning.
end subroutine get_library_dirs

!> Add link directories for all shared libraries in the dependency graph
Expand Down
Loading