Skip to content

fix: only resolve path variables for absolute DEPENDS entries#5352

Open
mvanhorn wants to merge 1 commit into
nasa:develfrom
mvanhorn:fix/3803-depends-resolves-local-files
Open

fix: only resolve path variables for absolute DEPENDS entries#5352
mvanhorn wants to merge 1 commit into
nasa:develfrom
mvanhorn:fix/3803-depends-resolves-local-files

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Jul 2, 2026

Copy link
Copy Markdown

Summary

fprime__process_module_setup ran resolve_path_variables over the entire DEPENDS list, which treated plain module dependency names as file paths and could mangle them when a local file of the same name existed.

Changes

For the DEPENDS control set, resolve path variables only for entries that are absolute paths; leave non-absolute entries (module names) untouched. All other control sets keep the existing behavior.

Testing

Added TestDependsCollision test deployment and a test_feature.py case exercising a module name that collides with a local file.

Fixes #3803

@thomas-bc thomas-bc requested a review from LeStarch July 2, 2026 18:09
@thomas-bc thomas-bc added the Breaking Changes / Needs Release Notes Need to add instructions in the release notes for updates. label Jul 2, 2026
@thomas-bc

Copy link
Copy Markdown
Collaborator

Need confirmation from @LeStarch that this is the intended behavior. I was taking an educated guess in the ticket comments, but you may have other insights?

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Coverage report — base devel

Overall (line): 81.39% → 81.39% (+0.00)
Regression threshold: 0.50% (line).

Regressions

(none over threshold)

Modules changed

Module Line Δ Function Δ Branch Δ
Os/Posix 62.00 -0.27 84.21 +0.00 43.87 -0.23

Modules without UTs

CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent

@LeStarch

LeStarch commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

I don't think this is the fix we want...as it will not attempt to resolve local paths. I don't know there is a solution outside of changing the DEPEND behavior like we did for SOURCES/AUTOCODER_INPUTS.

I'll close this for now.

@LeStarch LeStarch closed this Jul 9, 2026
@LeStarch LeStarch reopened this Jul 9, 2026
@LeStarch

LeStarch commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

I reopened this....as it caught my attention that this isn't just an AI PR slung at us. So let's think a bit more.

We have 2 options:

  1. We can split DEPENDS into two categories:

DEPENDS: dependencies on targets
LINKS: dependencies on files (.a and .so)

Where depends is used whole-sale, and LINKS resolves paths.

This is a clean, but breaking solution.

  1. We could fix this by checking if the DEPENDS is a target, resolving if it is NOT a target.

This isn't great because a target can be defined later on, and still collide with a same-named file....but it reduces the scope.

@mvanhorn thoughts?

@mvanhorn

Copy link
Copy Markdown
Author

Option 1 feels like the right destination. The root problem in #3803 is that DEPENDS overloads two meanings (target names and file paths), and every fix inside that ambiguity is a heuristic. Splitting the intent into DEPENDS (targets, used wholesale) and LINKS (files, path-resolved) removes the guessing entirely, and it's consistent with what you already did for SOURCES/AUTOCODER_INPUTS.

The breakage could be staged so it lands gently:

  1. Introduce LINKS with resolution semantics.
  2. Keep DEPENDS accepting file-ish entries for one release, but emit a deprecation warning when an entry contains a path separator or resolves to an existing file, pointing at LINKS.
  3. After the deprecation window, DEPENDS becomes targets-only.

Option 2 works as a transitional heuristic, but the later-defined-target collision you mention is the same class of ambiguity that produced #3803 - it just moves where the surprise happens, and it's order-dependent, which makes it hard to debug when it bites.

If you're good with direction 1, I'm happy to rework this PR into the LINKS split plus deprecation warning (or split it into two if you'd rather see LINKS land separately). If you'd prefer the smaller scope of option 2 for now, I can implement that instead with a docs note about the collision caveat.

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

Labels

Breaking Changes / Needs Release Notes Need to add instructions in the release notes for updates.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DEPENDS Will Resolve Local Files

3 participants