search

swiftlang / swift-package-manager

The Package Manager for the Swift Programming Language
Apache License 2.0
9.72k stars 1.34k forks source link

[Build/PackageGraph] Sink fallback logic for macro/plugin/test products and packages identification into `ModulesGraph` #7646

Closed xedin closed 3 months ago

xedin commented 3 months ago

This is a temporary fix until we can figure out a proper way to handle situations were all we get is a name of a product or a target.

Motivation:

Callers or ModulesGraph.{product, target}(for:destination:) cannot always know the right destination to use at the moment because i.e. for test products and targets its contextual. We need a proper fix for this at the level or BuildSystem but for now sinking the logic down into ModulesGraph is the safest option.

Modifications:

Result:

--target and --product options should work correctly regardless of underlying product/target kind.

Resolves: rdar://129400066

xedin commented 3 months ago

@swift-ci please test

xedin commented 3 months ago

@swift-ci please test

xedin commented 3 months ago

@swift-ci please test Windows platform

xedin commented 3 months ago

Yeah, I need to figure out where to put the tests for this…

xedin commented 3 months ago

@MaxDesiatov I added tests to cross-compilation suite and build operation. While doing that I had an idea:

  1. Change BuildSubset to use an optional for destination parameter and default that to .none
  2. Switch destination parameter of ModulesGraph.{product, target}(for:destination:) to use optional as well which is also defaulted to .none.
  3. This might make "check .destination first and fallback to .tools" better scoped - only applicable when a specific destination is not requested and lets all of the commands use the graph without guessing the destination when its not clear.

WDYT?

xedin commented 3 months ago

@swift-ci please test

MaxDesiatov commented 3 months ago

@MaxDesiatov I added tests to cross-compilation suite and build operation. While doing that I had an idea:

  1. Change BuildSubset to use an optional for destination parameter and default that to .none
  2. Switch destination parameter of ModulesGraph.{product, target}(for:destination:) to use optional as well which is also defaulted to .none.
  3. This might make "check .destination first and fallback to .tools" better scoped - only applicable when a specific destination is not requested and lets all of the commands use the graph without guessing the destination when its not clear.

WDYT?

SGTM

MaxDesiatov commented 3 months ago

@xedin would you be able to confirm that this issue is fixed in this PR? rdar://129400066

xedin commented 3 months ago

@MaxDesiatov Indeed it does!

xedin commented 3 months ago

🤦 forgot to stage private -> internal change last night for computeLLBuildName...

xedin commented 3 months ago

@swift-ci please test

xedin commented 3 months ago

@MaxDesiatov I'll follow-up with optional destination: changes.

xedin commented 3 months ago

@swift-ci please test Windows platform