Scope information in output array of BuildDependencyGraph()

[isc-cborbonm]

Description

• Closes BuildDependencyGraph() should include scope if dependency is scoped #1152: Adds information about scoped dependencies in the output array of BuildDependencyGraph()

Testing

1. New test modules under build-dependency-graph-scoped-dep
2. New test class Test.PM.Integration.BuildDependencyGraphScopedDeps:
   a. checks that non-scoped dependencies have an empty string in the dependency info list
   b. checks that scoped dependencies see their scope (namely, "test" for this example) in the dependency info list

[isc-dchui]

Can I ask what the context of this change is? What is the bigger picture change that this enables? (And why not bundle this change with that one?)

[isc-cborbonm]

Can I ask what the context of this change is? What is the bigger picture change that this enables? (And why not bundle this change with that one?)

Great question! The bigger picture is to allow a user who calls BuildDependencyGraph() to see at a glance which transitive dependencies are scoped and what their scope is (if one exists). The rest of the change isn't within IPM code itself but rather a different application that makes use of IPM (hence not bundling the change in this PR).

[isc-jili]

One typo found but otherwise looks great! @isc-cborbonm

[isc-kiyer]

@isc-cborbonm just the one small note. Looks good!

[isc-dchui]

Oh one more thing, to get GitHub to auto-link the issue, you must put the issue number immediately after the magic closing word, e.g. Closes #1152. ("Addresses" is not one of these magic words unfortunately.)

Of course, you're free to manually link the issue instead.

[isc-dchui]

Wait, why does this setting of pDependencyGraph have an extra argument of deployed in there? I know this predates your change, but I'm a bit confused by it.

[isc-cborbonm]

absolutely no clue 😃

[isc-dchui]

So I asked Claude to do some investigative journalism and here's what it found:

Dependency graph format inconsistency in BuildDependencyGraph

The pDependencyGraph array written by BuildDependencyGraph has three different
entry formats depending on which code path populated it:

Path	$list(data,1)	$list(data,2)	$list(data,3)	$list(data,4)	$list(data,5)	$list(data,6)
Already installed (line 1145)	depth	""	version	DisplayName	—	—
Exact-match from repo (line 1206)	depth	serverName	version	Deployed	PlatformVersion	DisplayName
Fuzzy-match from repo (line 1241)	depth	serverName	version	DisplayName	—	—

Root cause

This is a remnant of the iterative BFS rewrite in #975. The exact-match path was
ported from the old recursive implementation, which embedded Deployed and
PlatformVersion in the graph for downstream consumption. The new iterative design
no longer reads those positions back out of the graph (the inverted graph pipeline
only reads positions 2–3), so they became dead weight — but the format was never
cleaned up, leaving DisplayName at position 4 on two paths and position 6 on the
third.

Current impact

No active bug: ConstructInvertedDependencyGraph only reads $list(data, 2, 3), so
nothing downstream is broken today. However, any code added to read DisplayName
from the graph would silently get a Deployed boolean (0/1) for exact-match
dependencies.

@isc-kiyer what's your sense of the best thing to do here? This path-dependent inconsistency smells like a footgun and we should probably fully normalize it. Right now the caller has to know which path has been taken to know what the actual format will be.

[isc-kiyer]

@isc-dchui @isc-cborbonm Yeah I agree we should normalize it. I don't know anything downstream using the platform version and deployed flags but things are using display name. I would vote to remove those

[isc-cborbonm]

thanks @isc-kiyer ! updated the review to delete platform version and deployed flags

[isc-dchui]

Looks good! Thanks for addressing all the issues that popped up!