-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[SE-0480] Build settings for warning control flags #8315
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
Conversation
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the two places where we can go quadratic in the number of command-line arguments, this is looking good to me. But I'm not super familiar with the SwiftPM code base.
// `-w` (suppress warnings) and the other warning control flags are mutually exclusive | ||
for index in args.indices.reversed() { | ||
let arg = args[index] | ||
if arg.starts(with: "-W"), arg.count > 2 { | ||
// we consider the following flags: | ||
// -Wxxxx | ||
// -Wno-xxxx | ||
// -Werror | ||
// -Werror=xxxx | ||
// -Wno-error | ||
// -Wno-error=xxxx | ||
args.remove(at: index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop could be a filter
, which would make it linear-time rather than quadratic.
for index in args.indices.reversed() { | ||
let arg = args[index] | ||
switch arg { | ||
case "-warnings-as-errors", "-no-warnings-as-errors": | ||
args.remove(at: index) | ||
case "-Wwarning", "-Werror": | ||
guard args.indices.contains(index + 1) else { | ||
throw InternalError("Unexpected '\(arg)' at the end of args") | ||
} | ||
args.remove(at: index + 1) | ||
args.remove(at: index) | ||
default: | ||
break | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also technically quadratic. Even though it's a little ugly, I think I'd still rather do it with a filter
var removeNextArg = false
args = args.filter { arg in
if removeNextArg {
removeNextArg = true
return false
}
switch arg {
case "-warnings-as-errors", "-no-warnings-as-errors":
return false
case "-Wwarning", "-Werror":
removeNextArg = true
return false
default:
return true
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the quadratic complexity is an oversight on my part. Good thing you noticed.
TBH, using an external state in a filter makes me a bit uneasy. The documentation for filter
doesn't guarantee that the predicate will be invoked in any particular order, so I'd try to avoid it.
I don't insist though. If you think a filter here is fine, I don't mind. But what do you think about something more "manual", e.g:
var readIndex = args.startIndex
var writeIndex = args.startIndex
let endIndex = args.endIndex
while readIndex < endIndex {
let arg = args[readIndex]
switch arg {
case "-warnings-as-errors", "-no-warnings-as-errors":
break
case "-Wwarning", "-Werror":
readIndex += 1 // -Wwarning and -Werror have an argument
if readIndex >= endIndex {
throw InternalError("Unexpected '\(arg)' at the end of args")
}
default:
if readIndex != writeIndex {
args[writeIndex] = args[readIndex]
}
writeIndex += 1
}
readIndex += 1
}
if writeIndex < endIndex {
args.removeSubrange(writeIndex..<endIndex)
}
A couple of general questions. Placing these flags in the package manifest, means all consumers of this package will build with the same settings. I was just wondering if it's a concern that consumers may want to set them differently. For example, if they are specifying additional warning setting flags on the Also, as we're working on adopting Swift Build to replace our native build system, we would need to make sure this is handled there as well. |
Mind you the pitch and evolution proposal predates my participation so maybe this has already been addressed. I'll need to catch up. |
That's a good question. AFAIK, the additional flags (-Xcc, -Xcxx, -Xswiftc) are placed after the internally generated flags, but I didn't really check that. I'll dive into it later. If it's not the case, I think we should make it so. However, I need a confirmation from the maintainers that that's really the desired behavior.
I've raised a discussion about that, but it seems like the Swift Build maintainers prefer a simpler approach to these flags. swiftlang/swift-build#248
The original proposal (SE-0443) doesn't address this API, but we're working on fixing that. We will probably write a new pitch. |
@swift-ci Please test |
@swift-ci please test windows |
Following up on this question. The additional arguments the used provided are placed after the ones SwiftPM generates. So if we consider the following Package.swift: let package = Package(
name: "MyExecutable",
platforms: [
.macOS(.v13),
],
targets: [
.target(
name: "cfoo",
cSettings: [
.enableWarning(name: "all"),
.disableWarning(name: "unused-parameter"),
.treatAllWarnings(as: .error),
.treatWarning(name: "unused-variable", as: .warning),
]
),
.target(
name: "cxxfoo",
cxxSettings: [
.enableWarning(name: "all"),
.disableWarning(name: "unused-parameter"),
.treatAllWarnings(as: .error),
.treatWarning(name: "unused-variable", as: .warning),
]
),
.executableTarget(
name: "swiftfoo",
swiftSettings: [
.treatAllWarnings(as: .error),
.treatWarning(name: "DeprecatedDeclaration", as: .warning),
]
),
]
) And build it with additional arguments like this: swift-build --very-verbose -Xcc -Wno-error -Xswiftc -no-warnings-as-errors We will get the following invocations of the compilers:
Given that these flags are evaluated left-to-right it gives the user some control here at the build stage.
|
92fdcd3
to
a51a5de
Compare
@swift-ci Please test |
@swift-ci Please test macOS |
a51a5de
to
5bbc023
Compare
@swift-ci Please test |
@swift-ci Please test macOS |
@swift-ci Please test windows |
@swift-ci Please test |
@swift-ci Please test |
@swift-ci Please test |
@swift-ci Please smoke test |
@swift-ci Please clean smoke test |
@swift-ci Please smoke test |
@swift-ci Please test |
@swift-ci Please test macOS platform |
} | ||
|
||
func testWarningControlFlags() async throws { | ||
try XCTSkipOnWindows(because: "https://github.com/swiftlang/swift-package-manager/issues/8543: there are compilation errors") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: The references GitHub issue does not appear to be related to this test. Are we certain this test fails on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, both testManifestGenerationWithWarningControlFlags
and this test fail on Windows. And I think it is related to the issue. The CI job with the errors is already removed from the dashboard, so I can't provide you with the link, but it was showing the same type of errors as in the linked issue:
type 'CSetting' has no member 'enableWarning'
type 'SwiftSetting' has no member 'treatAllWarnings'
...
If I understand the issue right, for some reason, tests on Windows use the wrong version of PackageDescription
, not the one we build, so we don't see the newly added functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, do you recall if the failure occurred in the windows self hosted, windows smoke test, or both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it only fails in the windows self hosted builds. But I'm not certain.
I can remove the skips and rerun the tests if you want.
import _InternalTestSupport | ||
import XCTest | ||
|
||
final class PackageDescription6_2LoadingTests: PackageDescriptionLoadingTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Since this is a new test, consider migrating to Swift Testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I've marked it as an expected failure on Windows, so it will record the issue. A copy of the log is below
} | ||
|
||
func testManifestGenerationWithWarningControlFlags() async throws { | ||
try XCTSkipOnWindows(because: "https://github.com/swiftlang/swift-package-manager/issues/8543: there are compilation errors") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: The references GitHub issue does not appear to be related to this test. Are we certain this test fails on Windows?
@swift-ci Please test |
The error on Swift Test Windows Platform (self hosted) (Swift 6.1) Log
|
@swift-ci Please test Windows platform |
3 similar comments
@swift-ci Please test Windows platform |
@swift-ci Please test Windows platform |
@swift-ci Please test Windows platform |
@bkhouri can you review this again, please? Or tag someone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm more familiar with the SwiftPM tests.
@dschaefer2 would be in a better position to approve this PR than me.
results.checkIsEmpty() | ||
} | ||
} when: { | ||
isWindows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note (non-blocking): In SwiftPM, we can also use ProcessInfo.hostOperatingSystem == .windows
import _InternalTestSupport | ||
import Testing | ||
|
||
struct PackageDescription6_2LoadingTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: It's great to a a new test suite being written in Swift Testing. thanks for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still concerned we're changing the Package Manifest without having a cohesive vision for it that we can measure this feature against. Unfortunately that's vision is going to take some time to put together.
Approving since it was an accepted evolution proposal. But I reserve the right to adjust it in the future :). Big things I'm wondering about are things like build configuration and platform specific build settings, build setting visibility like CMake has, etc. Not sure the current build settings syntax scales to that level of optionality, but that needs to be studied.
This change adds warning control settings as described in the [proposal](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0480-swiftpm-warning-control.md). The flags for Swift targets are described by SE-0443, for Clang targets - [here](https://clang.llvm.org/docs/UsersManual.html#options-to-control-error-and-warning-messages).
This change adds warning control settings as described in the proposal. The flags for Swift targets are described by SE-0443, for Clang targets - here.