Skip to content

Fix LD_LIBRARY_PATH handling for shared libraries during fpm test#1259

Open
ZainabTravadi wants to merge 2 commits intofortran-lang:mainfrom
ZainabTravadi:fix-ld-library-path-tests
Open

Fix LD_LIBRARY_PATH handling for shared libraries during fpm test#1259
ZainabTravadi wants to merge 2 commits intofortran-lang:mainfrom
ZainabTravadi:fix-ld-library-path-tests

Conversation

@ZainabTravadi
Copy link
Copy Markdown
Contributor

When running fpm test for projects using shared libraries,
the compiled .so file may not be found because the directory
containing the library is not included in LD_LIBRARY_PATH.

This change ensures that shared library output directories
(target%output_dir) are included when collecting library
paths via get_library_dirs().

As a result, test executables can locate shared libraries
without requiring users to manually configure LD_LIBRARY_PATH.

Validation:

  • All existing fpm tests pass
  • Verified using a minimal fpm project with type="shared"
  • The test executable successfully located the .so
    without manually setting LD_LIBRARY_PATH

Copilot AI review requested due to automatic review settings March 12, 2026 18:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Ensures fpm test can locate shared libraries by including target output directories in the paths gathered by get_library_dirs() (so LD_LIBRARY_PATH includes the .so directory during test execution).

Changes:

  • Initializes shared_lib_dirs as empty and adds clarifying comments.
  • Filters considered targets in get_library_dirs() and adds target%output_dir while deduplicating.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/fpm_targets.f90 Outdated
Comment on lines 1386 to 1387
! 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.
Comment thread src/fpm_targets.f90
Comment on lines +1388 to +1389
! Always include the output_dir for shared libraries
! Avoid duplicates
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.
Comment thread src/fpm_targets.f90 Outdated
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.

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.
@ZainabTravadi
Copy link
Copy Markdown
Contributor Author

I updated the implementation to only consider FPM_TARGET_SHARED targets and replaced the condition with a clearer any() membership check as suggested. The comments were also updated to reflect that only shared libraries affect LD_LIBRARY_PATH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants