Plugin: Stop building with -enable-testing even with release config

[kateinoigakukun]

Motivation:

When building a SwiftPM target through PackagePlugin APIs and overall configuration is debug, they were built with -enable-testing even with plugins requests building with release configuration.

This mis-configuration led performance and code size regression only when building through package plugins.

The root problem is that some of the properties under BuildParameters store values derived from other properties.
buildParameters.testing.enableTestability was one of them. It depends on configuration but it's computed only in BuildParameters.Testing initializer. Therefore the property was not updated after PluginDelegate modifies configuration for requested builds.

swift-package-manager/Sources/Commands/Utilities/PluginDelegate.swift

Lines 118 to 127 in 23fa281

	switch parameters.configuration {
	case .debug:
	buildParameters.configuration = .debug
	case .release:
	buildParameters.configuration = .release
	case .inherit:
	// The top level argument parser set buildParameters.configuration according to the
	// --configuration command line parameter. We don't need to do anything to inherit it.
	break
	}

swift-package-manager/Sources/SPMBuildCore/BuildParameters/BuildParameters+Testing.swift

Line 107 in 23fa281

self.enableTestability = enableTestability ?? (.debug == configuration)

For those who want an immediate workaround: One of the workaround was to set the overall configuration as release, but it forces building plugins as release too.

Modifications:

This PR made those properties that depend on other stored properties as computed properties rather than stored ones.

There are still some properties having the same issue (BuildParameters.Debugging), so I'll fix them in a follow-up PR separately.

Result:

Release builds requested through package plugins are truly built as release mode even if the overall configuration is debug.

[kateinoigakukun]

@swift-ci test

[MaxDesiatov]

@swift-ci test

[grynspan]

I haven't dug in but I would suggest making sure the flag is still set when calling --build-tests or swift test since the intent there is pretty clear.

[kateinoigakukun]

@grynspan That's a good point. I added a validation test for swift test -c release. For swift build --build-tests -c release, I'll make a follow-up fix separately because it has not been working for long and is independent from plugin system.

[kateinoigakukun]

@swift-ci test

[MaxDesiatov]

@swift-ci test windows

[grynspan]

@plemarquand FYI

[dschaefer2]

Wouldn't changing flags with swift test, or --build-tests, trigger an expensive full rebuild?

[kateinoigakukun]

@dschaefer2 No full rebuilding triggered between swift build, swit build --build-tests, and swift test. Changing between swift build -c release and swift test -c release triggers a full rebuild but I think it's acceptable to provide the best performance for release mode.
For those who don't really want full rebuilding even with release mode, they can use swift build -c release --enable-testable-imports and swift test -c release instead to take build time over runtime performance.

[dschaefer2]

The community is very sensitive to build times, especially when macros are involved as is the case with swift-testing. And the worst case is building SwiftSyntax in release mode which can take up to 5 minutes.

[kateinoigakukun]

@dschaefer2 First of all, this PR does not change the build behavior triggered by the CLI but changes the build behavior triggered through Package Plugin APIs to be aligned with today's CLI's behavior.

Back to the build time tradeoff, I agree that today's CLI behavior is arguable. I think we have three options:

1. Keep the current behavior
   • Changing testability always requires full rebuilding
2. Have separate build description and object directory for with/without testability, not to overwrite artifacts
   • Changing testability requires full rebuilding only for the first time
   • Need non-trivial refactoring in SwiftPM because we assume a pair of build description and object directory corresponds to a configuration.
     • Or internally introduce [debug, debug-test, release, release-test] configurations
   • Double disk footprint if switching testability even once
3. Always build with -enable-testing
   • Changing testability never requires full rebuilding
   • Sacrifice runtime performance and code size in release mode

I think the current behavior is a good balance option given that most developers rarely use swift test -c release nor swift build -c debug --disable-testable-imports, but I'd like to hear your opinion here

[grynspan]

If we do continue with this change, let's make sure to run a full toolchain build against it.

[kateinoigakukun]

I verified this change doesn't work full toolchain build :)

Since 1. this change does not change anything in CLI behavior but just for package plugin invocations (not related to macro plugins, to be clear), 2. also it just aligns with the current CLI behavior, and 3. we are suffering from a huge performance regression due to this issue, I'd like to proceed this change and discuss the incremental build topic as a separate issue (#8031)
Does it make sense for you? @MaxDesiatov @dschaefer2

[grynspan]

"Doesn't work" or "doesn't break"? 😬

[kateinoigakukun]

"Doesn't break"😅

[MaxDesiatov]

LGTM, but I hope @xedin could have a look before this is merged

[MaxDesiatov]

cc @rauhul who's building in release configuration for embedded

[xedin]

Makes sense to me for both to be consistent. Once it is decided what release behavior in regards to testability should be (@rauhul has a thread about that on forums) we could change both places to reflect that.

[dschaefer2]

Agreed.