Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep separate build parameters for host and target #7164

Merged
merged 13 commits into from
Dec 7, 2023

Conversation

MaxDesiatov
Copy link
Contributor

@MaxDesiatov MaxDesiatov commented Dec 5, 2023

Unblocks support for macros in cross-compilation implemented in #7118.

Motivation:

So far we used a single BuildParameters value for the whole build graph, which explicitly mentioned both host and target triples, but assumed you can use the same toolchain for both, which is not true. Additionally, it would assume that all other build parameters can be shared between the host and target, like debug info format, build configuration, sanitizers etc. That's not true either, one would use CodeView for build tools on Windows when cross-compiling products with DWARF for Linux, or build macros in release mode for performance while cross-compiling code that uses debug build configuration.

Modifications:

Removed hostTriple and targetTriple from BuildParameters, keeping only a single triple property of Triple type in it. In SwiftTool and BuildPlan we'll use two separate destinationBuildParameters and toolsBuildParameters instead. The latter will pick one of the two BuildParameters values for specific target and product build descriptions.

Result:

We can compile build tools for the host, while cross-compiling end products for the target, in the same build graph. This also removes one of big roadblocks in folding build tool and package plugins into the build graph, instead of building and invoking them separately outside of the build graph.

`BuildParameters` to date explicitly mentioned both host and target triples, but assumed you can use the same toolchain for both, which is not true. Additionally, it would assume that all other build parameters can be shared between the host and target, like debug info format, build configuration, sanitizers etc. That's not true either, one would use CodeView for build tools on Windows when compiling targets with DWARF for Linux, or build macros in release mode for performance when cross-compiling code that uses debug configuration itself.
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov added the build system Changes to interactions with build systems label Dec 5, 2023
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov MaxDesiatov marked this pull request as ready for review December 6, 2023 16:18
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

Copy link
Contributor

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

looks go to me, but not an expert in the build module. the names "buildTools" and "buildProducts" are a bit confusing (new terminology) and would prefer to use "host" and "target" (or similar) if possible

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Dec 6, 2023

looks go to me, but not an expert in the build module. the names "buildTools" and "buildProducts" are a bit confusing (new terminology) and would prefer to use "host" and "target" (or similar) if possible

IMO we're trading one confusion for another, the main question is which is more confusing, as we're mixing up triple targets and build system targets in the same context. For example, we have TargetBuildDescription type in the build system sense, and would have targetBuildParameters in the triples sense as well, all in the same module. IMO that situation is even more confusing. @neonichu WDYT?

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@neonichu
Copy link
Contributor

neonichu commented Dec 6, 2023

I don't have a strong opinion, but looking at our CLI, we currently are using these terms here:

  • "build tools" for content we need during build time (e.g. -Xbuild-tools-swiftc)
  • "destination" and "Swift SDK" for selecting the destination/target

So at least "build tools" seems well established at this point.

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Dec 6, 2023

So at least "build tools" seems well established at this point.

Right, and buildToolsBuildParameters seems tautological, hence just toolsBuildParameters.

The main question is about "target" naming. moduleBuildParameters seems too ambiguous, so unless we find a better name I propose sticking to productsBuildParameters or maybe endProductsBuildParameters for a bit more clarity?

@rauhul
Copy link
Member

rauhul commented Dec 6, 2023

IMO targetBuildParameters and hostBuildParameters isn't too difficult to follow; maybe targetPlatform and hostPlatform could be more clear?

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Dec 6, 2023

IDK, at a glance it doesn't seem obvious whether targetPlatform is a platform that a build plan target is compiled for, or a platform that will run the end product. You have to spend a good amount of time in this code to gain an intuition of its terminology, and it's already quite convoluted and poorly documented.

To make things worse, our build plan and package graph targets don't even correspond to actual llbuild targets either. 🥲

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

MaxDesiatov commented Dec 7, 2023

After sleeping on it, I'm leaning towards toolsBuildParameters and destinationBuildParameters. The latter makes it consistent with Xcode and avoids ambiguity until we potentially clean "target" naming in the build planning code. Thank you @neonichu for reminding me about "destinations"! 🙂

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) December 7, 2023 11:37
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

…xd/split-build-parameters

# Conflicts:
#	Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift
#	Tests/BuildTests/BuildPlanTests.swift
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

2 similar comments
@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov
Copy link
Contributor Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit cf727a8 into main Dec 7, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/split-build-parameters branch December 7, 2023 19:55
@rauhul
Copy link
Member

rauhul commented Dec 7, 2023

regardless of naming, im super excited to see this land!

@MaxDesiatov
Copy link
Contributor Author

Thanks! It's early to celebrate, this one's technically an NFC. We still need to sort out things in PackageGraph to assign build parameters to targets correctly, and I'm not done with that yet 😕

euanh added a commit to euanh/swift-package-manager that referenced this pull request Jan 22, 2024
…the target

Since swiftlang#7164, dependencies of command plugins are once again being
built for the _target_ rather than the host.   This causes problem
when cross compiling because the host needs to be able to run the
plugin dependencies, but finds target binaries instead.

This problem was fixed before in swiftlang#6791 by forcing command plugin
dependencies to be built for the host by overriding the default
build parameters in swiftTool.createBuildSystem().  The same solution
still works in this commit, but a better long-term option would be
to rework BuildOperation.plan() to handle command plugin dependencies
specially, as it already does for build plugin dependencies.

At present, BuildOperation.plan calls graph.invokeBuildToolPlugins to
process sources.  invokeBuildToolPlugins finds all build tool dependecies
and builds them separately, using a specially-created BuildOperation instance:

    https://github.com/apple/swift-package-manager/blob/34efc0bfe9d40d9a019644ac8fcd0b852c491dfe/Sources/SPMBuildCore/Plugins/PluginInvocation.swift#L409

There is no equivalent step for command plugin dependencies, so
they are built for the host architecture.   Ideally we should rework
BuildOperation.plan to build command and build plugin dependencies
in the same way.
euanh added a commit to euanh/swift-package-manager that referenced this pull request Jan 22, 2024
…the target

Since swiftlang#7164, dependencies of command plugins are once again being
built for the _target_ rather than the host.   This causes problem
when cross compiling because the host needs to be able to run the
plugin dependencies, but finds target binaries instead.

This problem was fixed before in swiftlang#6791 by forcing command plugin
dependencies to be built for the host by overriding the default
build parameters in swiftTool.createBuildSystem().  The same solution
still works in this commit, but a better long-term option would be
to rework BuildOperation.plan() to handle command plugin dependencies
specially, as it already does for build plugin dependencies.

At present, BuildOperation.plan calls graph.invokeBuildToolPlugins to
process sources.  invokeBuildToolPlugins finds all build tool dependecies
and builds them separately, using a specially-created BuildOperation instance:

    https://github.com/apple/swift-package-manager/blob/34efc0bfe9d40d9a019644ac8fcd0b852c491dfe/Sources/SPMBuildCore/Plugins/PluginInvocation.swift#L409

There is no equivalent step for command plugin dependencies, so
they are built for the host architecture.   Ideally we should rework
BuildOperation.plan to build command and build plugin dependencies
in the same way.
euanh added a commit to euanh/swift-package-manager that referenced this pull request Jan 22, 2024
…the target

Since swiftlang#7164, dependencies of command plugins are once again being
built for the _target_ rather than the host.   This causes problem
when cross compiling because the host needs to be able to run the
plugin dependencies, but finds target binaries instead.

This problem was fixed before in swiftlang#6791 by forcing command plugin
dependencies to be built for the host by overriding the default
build parameters in swiftTool.createBuildSystem().  The same solution
still works in this commit, but a better long-term option would be
to rework BuildOperation.plan() to handle command plugin dependencies
specially, as it already does for build plugin dependencies.

At present, BuildOperation.plan calls graph.invokeBuildToolPlugins to
process sources.  invokeBuildToolPlugins finds all build tool dependecies
and builds them separately, using a specially-created BuildOperation instance:

    https://github.com/apple/swift-package-manager/blob/34efc0bfe9d40d9a019644ac8fcd0b852c491dfe/Sources/SPMBuildCore/Plugins/PluginInvocation.swift#L409

There is no equivalent step for command plugin dependencies, so
they are built for the host architecture.   Ideally we should rework
BuildOperation.plan to build command and build plugin dependencies
in the same way.
euanh added a commit that referenced this pull request Jan 23, 2024
…the target (#7280)

Always build command line plugin dependencies for the host triple.

### Motivation:

Since #7164, dependencies of command plugins are once again being built
for the _target_ rather than the host. This causes problem when cross
compiling because the host needs to be able to run the plugin
dependencies, but finds target binaries instead.

This problem was fixed before in #6791 by forcing command plugin
dependencies to be built for the host by overriding the default build
parameters in swiftTool.createBuildSystem(). The same solution still
works in this commit, but a better long-term option would be to rework
BuildOperation.plan() to handle command plugin dependencies specially,
as it already does for build plugin dependencies.

### Modifications:

At present, BuildOperation.plan calls graph.invokeBuildToolPlugins to
process sources. invokeBuildToolPlugins finds all build tool dependecies
and builds them separately, using a specially-created BuildOperation
instance:


https://github.com/apple/swift-package-manager/blob/34efc0bfe9d40d9a019644ac8fcd0b852c491dfe/Sources/SPMBuildCore/Plugins/PluginInvocation.swift#L409

There is no equivalent step for command plugin dependencies, so they are
built for the host architecture. Ideally we should rework
BuildOperation.plan to build command and build plugin dependencies in
the same way. This commit forces all plugin dependencies to be built for
the host - this is similar to what was done in #6791 and #7273.
 
### Testing:
  
An integration test checks that any targets depended on by a command plugin
are built for the host, not for the target.
    
  * A new CommandPluginTestStub plugin has a dependency on a target executable
    which will be built automatically when the plugin is run.   The test checks that the
    dependency is built for the host architecture, no matter which target architecture
    is selected using '--triple'.
    
    * The plugin also asks SwiftPM to build the 'placeholder' main target.   The test
       checks that the dependency is built for the target architecture.
    
The test is restricted to macOS because we can be sure of having a viable
cross-compilation environment (arm64 to x86_64 and vice versa).  The standard
Linux build environments can't cross compile to other architectures.

### Result:

Command plugins can be used again when cross-compiling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug build system Changes to interactions with build systems cross-compilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants