Update Target Based Resolution #3838

SDGGiesbrecht · 2021-11-05T05:28:01Z

Associated pull requests:

See this comment in #3829 for the impetus.

I have brought all but two tests up to date, and now I have questions about them. (For one of @tomerd, @abertelrud or @neonichu?)

FunctionalTests.PluginTests/testUseOfBuildToolPluginProductByExecutableAcrossPackages is trying to launch a plugin executable before building it. Can someone point me roughly to where or how SwiftPM communicates to llbuild that one command cannot start until another has finished? (All the necessary commands seem to be there, just not not in the right order.)
XCBuildSupportTests.PIFBuilderTests/testTestProducts is objecting with “failed to retrieve search paths with pkg-config; maybe pkg-config is not installed”. I do not know whether that is a real failure or just due to the machine I am using. Can someone else confirm one way or the other?

(I will list and explain all the other updates once I am finished; all are minor and self‐contained.)

abertelrud · 2021-11-05T21:00:55Z

FunctionalTests.PluginTests/testUseOfBuildToolPluginProductByExecutableAcrossPackages is trying to launch a plugin executable before building it. Can someone point me roughly to where or how SwiftPM communicates to llbuild that one command cannot start until another has finished? (All the necessary commands seem to be there, just not not in the right order.)

Looks as if this plugin depends on an executable built from another target, and that is the one that's failing to build first? The plugin dependencies use the normal dependencies machinery underneath, so in theory this should already provide the right dependencies. I'll take a closer looks and see if I have some advice.

XCBuildSupportTests.PIFBuilderTests/testTestProducts is objecting with “failed to retrieve search paths with pkg-config; maybe pkg-config is not installed”. I do not know whether that is a real failure or just due to the machine I am using. Can someone else confirm one way or the other?

I thought that pkg-config was handled through a custom parser rather than invoking the actual pkg-config tool, so this puzzles me. I can't confirm but will take a look at this as well.

SDGGiesbrecht · 2021-11-05T21:33:51Z

Thanks, @abertelrud.

I have already been debugging the plugin one some more.

I have discovered that computeDependencies(of: ResolvedProduct) seems never to be called for any plugin product, even when it is in the root package. This looks like the line that filters them out beforehand. I suspect in‐package plugins work okay, because the targets and their interdependencies are intact, even though no product is ever “built”. It might be correct to “build nothing”, but culling the node there is also culling its transitive dependencies. At the moment I am experimenting with deferring the culling to a later stage, but I keep getting other errors because at present, later code does not expect plugins and triggers unrelated fatal errors. I will get back to you when I have investigated more thoroughly.

SDGGiesbrecht · 2021-11-05T23:53:04Z

That was probably a wild goose chase. Adding the plug‐in tool product as a dependency in addition to the plug‐in script restores the missing links in the chain and llbuild knows what to do. But it does not look like that extra declaration is supposed to be necessary. Back to square one.

SDGGiesbrecht · 2021-11-06T06:13:50Z

I figured out the problem with the plug‐ins. The part that creates the executable binary comes from the product, not the target. Since the declared dependencies go client → plugin‐script‐product → plugin‐script‐target → executable‐target, there is no executable‐product in the graph. I fixed it by synthesizing a product for any executable that a plug‐in script depends on (funnelling it into the same code that does the synthesis for executable targets in the root package). I am doing further tests now, and will commit it once I know whether it has any unexpected side‐effects.

(After figuring that out, I went back to main, removed the explicit product from the test fixture, and ran the test. It failed in the same way with the same cryptic message. So it will need fixing even if you decide not to keep any of this target‐based stuff.)

SDGGiesbrecht · 2021-11-06T08:02:00Z

In addition to the pkd-config puzzle, I have noticed some FIXMEs I would like to look closer at, because they claim to be related to target based resolution. I will probably get to it next week.

SDGGiesbrecht · 2021-11-08T09:26:40Z

I went searching in the wild until I found a broken package and then traced it through the resolver. PubGrubPackageContainer’s private cache was indexing incorrectly, and it was capable of derailing things downstream in certain situations. My latest commit fixed it and added a test.

Reading back over the earlier threads, this bug was probably the cause of the issues @neonichu and I were trying to solve months ago. The symptoms certainly match.

At this point, it would be helpful to know how this fares against smoke testing and the source compatibility suite.

abertelrud · 2021-11-08T16:07:54Z

I figured out the problem with the plug‐ins. The part that creates the executable binary comes from the product, not the target. Since the declared dependencies go client → plugin‐script‐product → plugin‐script‐target → executable‐target, there is no executable‐product in the graph. I fixed it by synthesizing a product for any executable that a plug‐in script depends on (funnelling it into the same code that does the synthesis for executable targets in the root package). I am doing further tests now, and will commit it once I know whether it has any unexpected side‐effects.

(After figuring that out, I went back to main, removed the explicit product from the test fixture, and ran the test. It failed in the same way with the same cryptic message. So it will need fixing even if you decide not to keep any of this target‐based stuff.)

Wow, thank you for digging into this and finding this! I'll take a look at the specifics of what you had to do with synthesizing a product, but it sounds like the right approach — I actually thought that all packages already had this synthesis. The only thing to watch out for is that dependencies shouldn't have access to the synthesized executables if the package author didn't intentionally make them public.

The product vs target distinction did cause a certain amount of grief when the plugin support was added (as it also did when testable executable modules were addded).

(By the way, the product vs target distinction may have seemed like a good idea initially but at this point costs everyone more than it's worth, and I think is worth revisiting at some point — but that is obviously a later topic for a discussion and proposal.)

abertelrud · 2021-11-08T16:08:03Z

@swift-ci please smoke test

abertelrud · 2021-11-08T16:27:44Z

Sources/PackageGraph/PackageGraph+Loading.swift

-        packageBuilder.dependencies = Array(dependencies.values)
+        packageBuilder.dependencies = dependencies.values
+            // Restore determinism:
+            .sorted(by: { $0.package.identity < $1.package.identity })


Sorting these for stability is great, I think. In the long run I think it would actually be nice if we could get back to using the order in which dependencies were declared in the manifest. It shouldn't make a difference to the logic of course, but it seems to me that using the user's given order would be more natural to read as a package author. It's a total detail, just something I hope that SwiftPM can get back to at some point (I think it used to do this early on).

I thought about that too, and considered refactoring to use an OrderedDictionary. But pulling in swift-collections seemed too far off‐topic for this PR. It is certainly something to consider in the future.

Absolutely. It was more of a comment about a general approach here. I should have made that clear.

abertelrud · 2021-11-08T19:15:37Z

Sources/PackageLoading/PackageBuilder.swift

+        let implicitPlugInExecutables = Set(targets.lazy
+             .filter({ $0.type == .plugin })
+             .flatMap({ $0.dependencies })
+             .map { $0.name })


If I read this correctly, this prevents non-plugin targets from depending on these implicit products (which would be great). Or am I reading that wrong?

Probably...? It certainly means they are not there to depend on during resolution or loading. But later when the build system is constructed there is no insulation to prevent dangling references from pointing where it will end up. What I do not know for sure, is if there is a way for it to sneak under the diagnostic checks to get that far, since some dangling references are temporarily permitted so that later diagnostics have more information available to suggest fixes.

Thanks for clarifying — it's very possible that we should (in a separate PR, by me) add more checks for that kind of thing.

Representing plugins as targets definitely has some trade-offs. It's a bit odd to do so, but making them completely separate would have made it more difficult to fit them into the dependency graph. I still think it would be even more complicated to make them participate in the dependency graph without being targets, but it definitely does introduce some edge cases.

SDGGiesbrecht · 2021-11-08T21:01:49Z

@abertelrud,

The only thing to watch out for is that dependencies shouldn't have access to the synthesized executables if the package author didn't intentionally make them public.

To make sure I understand what you mean, let me ask it like this: Should the following work or not, assuming the intent is to have SwiftPM to build PlugInExecutableTarget and launch it like PlugInExecutableTarget some-argument --some-flag?

// Dependency
let package = Package(
  // ...
  products: [
    .plugin(name: "PlugInScriptProduct", targets: ["PlugInScriptTarget"]),
    // (There is no product for “PlugInExecutableTarget”.)
  ],
  targets: [
    .plugin(
      name: "PluginScriptTarget",
      // ...
      dependencies: ["PlugInExecutableTarget"]
    ),
    .executableTarget(name: "PlugInExecutableTarget")
  ]
)

// Root
let package = Package(
  // ...
  targets: [
    .target(
      // ...
      plugins: [
        .plugin(name: "PlugInScriptProduct", package: "Dependency")
      ]
    )
  ]
)

SDGGiesbrecht · 2021-11-08T21:05:59Z

Well the CI log tells me half of the pkg-config issue is real, since CI did not wine about it the installation the way it does on my machine, but it did hit other trouble later. I will keep investigating.

SDGGiesbrecht · 2021-11-08T23:00:15Z

I figured out the pkg‐config issue. The in-memory file system just lacked the .pc file. The test probably just fell out of date while it was disabled.

Looking over other FIXME now.

SDGGiesbrecht · 2021-11-09T02:48:11Z

That last commit fixes a bug where packages with tools version from 5.2 to 5.4 that had a name property different from the last path component behaved like 5.1 manifests do, asking the resolver to look for more than necessary. This was because the identity work broke the lookup from the target dependency to the package dependency, and the manifest could not tell which package the product was supposed to belong to. Due to having no known connection, the logic was landing in the code branches intended for resolving 5.1 manifests.

Locally all tests are green and every package have tried now seems to be working as expected. Let me know:

if you find any other problems
if you want this PR rebased, split up, or otherwise adjusted
what you want done with ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION:
- remove the checks entirely?
- default to on like the PR’s current state (so the central switch remains for a while in case problems arise)?
- default to off like it has been for the last while?
- or some other plan?

neonichu · 2021-11-09T18:35:10Z

Thanks for your work, @SDGGiesbrecht!

I think leaving ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION in for a bit, but turned on sounds like the best option to me for this PR. Once we're confident, we can remove it entirely in a future PR.

SDGGiesbrecht · 2021-11-09T20:08:53Z

I adjusted the manifest so that you can set DISABLE_TARGET_BASED_DEPENDENCY_RESOLUTION in the environment to flip the compilation condition on or off. By default, the feature is enabled.

abertelrud · 2021-11-09T23:24:41Z

@abertelrud,

The only thing to watch out for is that dependencies shouldn't have access to the synthesized executables if the package author didn't intentionally make them public.

To make sure I understand what you mean, let me ask it like this: Should the following work or not, assuming the intent is to have SwiftPM to build PlugInExecutableTarget and launch it like PlugInExecutableTarget some-argument --some-flag?
// Dependency
let package = Package(
  // ...
  products: [
    .plugin(name: "PlugInScriptProduct", targets: ["PlugInScriptTarget"]),
    // (There is no product for “PlugInExecutableTarget”.)
  ],
  targets: [
    .plugin(
      name: "PluginScriptTarget",
      // ...
      dependencies: ["PlugInExecutableTarget"]
    ),
    .executableTarget(name: "PlugInExecutableTarget")
  ]
)
// Root
let package = Package(
 // ...
 targets: [
   .target(
     // ...
     plugins: [
       .plugin(name: "PlugInScriptProduct", package: "Dependency")
     ]
   )
 ]
)

Thanks for laying this out in such detail. Yes, this should work, since it's the plugin inside the package that depends on an executable inside the package. What I think should not happen would be for PlugInExecutableTarget to be accessible from the root package (by virtue of being reachable through a plugin).

abertelrud · 2021-11-09T23:26:20Z

Thanks for your work, @SDGGiesbrecht!

I agree, thank you very much @SDGGiesbrecht!

I think leaving ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION in for a bit, but turned on sounds like the best option to me for this PR. Once we're confident, we can remove it entirely in a future PR.

This seems reasonable to me we as well.

SDGGiesbrecht · 2022-03-05T01:59:18Z

what error do you see?

The expected errors vanish, and this warning appears instead:

dependency 'org.foo' is not used by any target

The manifest does not think any target uses org.foo, because its nameForTargetDependencyResolutionOnly is org.foo which is not a case‐sensitive match for the foo in the reference. See the source here.

This might also be broken even in legacy resolution. Right now the test might only be succeeding because it happens to be in a root manifest, which bypasses lookup altogether (because in legacy it does not need to figure out its product filters). Transitive manifests still filter through packageDependency(referencedBy:). See the source here.

tomerd · 2022-03-07T17:10:36Z

there are two dependencies in play here: registry one (.registry(identity: "org.foo", requirement: .upToNextMajor(from: "1.0.0"))) with identity org.foo and source control one (.sourceControl(url: "https://git/org/foo", requirement: .upToNextMajor(from: "1.0.0"))) with identity foo - so the target dependency lookup with foo should be valid unless they are de-duped, which is what happens when the test enables the de-duplication modes. it may be the case that the error message is wrong or different, we can try to fine tune it.

SDGGiesbrecht · 2022-03-07T22:08:47Z

valid unless they are de-duped, which is what happens when the test enables the de-duplication modes

The de‐duplication you speak of does not seem to be happening at a low enough level. Nothing in Manifest’s reference lookup is aware of these different modes.

But now that I know understand how it is intended to work, I think I will be able to find and sink the logic in order to fix it. Thank you.

SDGGiesbrecht · 2022-03-07T23:13:13Z

Never mind, I see now that the manifest is being completely overwritten and thus does not need to know about any of the higher level stuff.

I also neglected to notice earlier that the .disabled mode gets tested first, so all the print statements and break points I was looking at were under the opposite mode. 🤦 It was a red herring.

I am now pretty sure everything is working properly, and the test failure was just because the message changed along with the particular downstream error. Both the old and new errors indicate that it successfully navigated the stuff the test is about before any problems occurred.

This reverts commit 6cf85c5, reversing changes made to 45fdf77.

SDGGiesbrecht · 2022-07-13T06:20:10Z

@elsh, would you double check this last commit for me? These two tests started failing after #5635. Part of the tests’ dependency graphs is unreachable, and thus only present under legacy resolution. Prior to your recent update, I had simply added the missing links necessary to ensure the entire graph was reachable. But since the update, that no longer works.^† So my latest commit instead just adapts the expectations to what the current manifests are actually asking for. What I want to know from you is if the tests still test what they are supposed to, or if the tests become meaningless because the relevant part was in the unreachable portion of the dependency graph that is now skipped entirely.

SDGGiesbrecht · 2023-03-06T21:34:23Z

Closing. This and related PRs are stale and I do not have time to maintain them anymore. I will leave the related branches around for now in case anyone else wants to pick them up.

SDGGiesbrecht added 2 commits November 3, 2021 21:17

Enabled improved resolution.

29f37ca

Fixed most tests.

9eb7447

Fixed testUseOfBuildToolPluginProductByExecutableAcrossPackages.

100ea8f

Fixed PubgrubPackageContainer cache.

7ea7e4d

abertelrud reviewed Nov 8, 2021

View reviewed changes

abertelrud self-assigned this Nov 8, 2021

Fixed testTestProducts.

04d2d3b

Fixed leaky product requests.

2330a7b

SDGGiesbrecht marked this pull request as ready for review November 9, 2021 02:49

SDGGiesbrecht requested review from friedbunny, neonichu and tomerd as code owners November 9, 2021 02:49

Adjusted manifest to permit switching by envirnoment.

508f3ac

Merge branch 'master' into resolution

0edcbdd

SDGGiesbrecht added 12 commits March 7, 2022 17:15

Fixed remaining test.

46a44b0

Merge branch 'master' into resolution

aede970

Merge branch 'master' into resolution

7121cce

Merge branch 'master' into resolution

13f8cec

Merge branch 'master' into resolution

885456a

Merge branch 'master' into resolution

e243964

Added CI.

0374ad6

Disabled some broken tests.

dcf5c44

Merge branch 'master' into resolution

45fdf77

Merge branch 'master' into ci

04c9a3c

Merge branch 'ci' into resolution

6cf85c5

Revert "Merge branch 'ci' into resolution"

4527994

This reverts commit 6cf85c5, reversing changes made to 45fdf77.

SDGGiesbrecht mentioned this pull request May 18, 2022

Update Target Based Resolution (CI) SDGGiesbrecht/swift-package-manager#1

Closed

SDGGiesbrecht added 6 commits May 18, 2022 20:21

Fixed test.

43bbbfb

Fixed test.

1c513ca

Resolved conflicts.

19b894b

Fixed module alias tests.

0f092d7

Overwrote conflicts from master.

c7445f0

Fixed test expectations.

1f27f39

SDGGiesbrecht added 3 commits July 13, 2022 15:28

Merge branch 'master' into resolution

8a61a4a

Merge branch 'master' into resolution

91b9b0b

Blocked setting from system libraries.

195741a

SDGGiesbrecht closed this Mar 6, 2023

Update Target Based Resolution #3838

Update Target Based Resolution #3838

Uh oh!

Conversation

SDGGiesbrecht commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud commented Nov 5, 2021

Uh oh!

SDGGiesbrecht commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SDGGiesbrecht commented Nov 5, 2021

Uh oh!

SDGGiesbrecht commented Nov 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SDGGiesbrecht commented Nov 6, 2021

Uh oh!

SDGGiesbrecht commented Nov 8, 2021

Uh oh!

abertelrud commented Nov 8, 2021

Uh oh!

abertelrud commented Nov 8, 2021

Uh oh!

abertelrud Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

SDGGiesbrecht Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

SDGGiesbrecht Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

SDGGiesbrecht commented Nov 8, 2021

Uh oh!

SDGGiesbrecht commented Nov 8, 2021

Uh oh!

SDGGiesbrecht commented Nov 8, 2021

Uh oh!

SDGGiesbrecht commented Nov 9, 2021

Uh oh!

neonichu commented Nov 9, 2021

Uh oh!

SDGGiesbrecht commented Nov 9, 2021

Uh oh!

abertelrud commented Nov 9, 2021

Uh oh!

abertelrud commented Nov 9, 2021

Uh oh!

SDGGiesbrecht commented Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomerd commented Mar 7, 2022

Uh oh!

SDGGiesbrecht commented Mar 7, 2022

Uh oh!

SDGGiesbrecht commented Mar 7, 2022

Uh oh!

SDGGiesbrecht commented Jul 13, 2022

Uh oh!

SDGGiesbrecht commented Mar 6, 2023

Uh oh!

Uh oh!

SDGGiesbrecht commented Nov 5, 2021 •

edited

Loading

SDGGiesbrecht commented Nov 5, 2021 •

edited

Loading

SDGGiesbrecht commented Nov 6, 2021 •

edited

Loading

SDGGiesbrecht commented Mar 5, 2022 •

edited

Loading