[presets] Actually make https://github.com/swiftlang/swift/pull/81284 work

[AnthonyLatsis]

--extra-swift-cmake-options=-DCMAKE_CXX_FLAGS=<flags> overwrites the variable rather than appends to it.

[AnthonyLatsis]

@swift-ci please smoke test

[etcwilde]

I'm not sure how I feel about this. If you're overriding a variable, you should override it. If we want to change the behavior of how the flag works in build-script, it should stay in build-script. But then we will also need a separate mechanism for removing and replacing values as well.

[etcwilde]

I wonder if we should move this to the buildbot_incremental_base preset? The smoketest preset only gets run if someone explicitly asks for a smoke test, but not if they only ask for PR testing.

[etcwilde]

Alternatively, it might be good to spell out the flags because at this point, I have no idea what the existing flags are, or where they're coming from. These preset files don't make it easy to track down where flags are coming from.

[AnthonyLatsis]

Alternatively, it might be good to spell out the flags because at this point

Sorry, by spell out the flags you mean...?

[AnthonyLatsis]

I wonder if we should move this to the buildbot_incremental_base preset?

I would rather exclude the possibility of some recurrent no-asserts job accidentally picking this up. Ideally, this should affect only the presets that can be used to fulfill our PR status checks. So perhaps a better alternative is to stick it in a pr-testing preset and apply that to the nodes we use for that purpose.

[AnthonyLatsis]

I'm not sure how I feel about this. If you're overriding a variable, you should override it. If we want to change the behavior of how the flag works in build-script, it should stay in build-script. But then we will also need a separate mechanism for removing and replacing values as well.

I don’t think special-casing option merging for CMAKE_CXX_FLAGS in build-script-impl is any better than this. You may want to override the variable as well as append to it.

[compnerd]

CMake recommendations are that CMAKE_C_FLAGS, CMAKE_CXX_FLAGS, CMAKE_Swift_FLAGS are off limits for CMakeLists.txt. If we want to allow the user to specify additional flags, that should require a special option e.g. -D Swift_ADDITIONAL_C_FLAGS=-Werror=unused -D Swift_ADDITIONAL_CXX_FLAGS=-Werror-unused -D Swift_ADDITIONAL_Swift_FLAGS="-Xcc -Werror-unused".

[compnerd]

To be clear, the options should be added via compile_options at the top level, not by changing the CMAKE_{C,CXX,Swift}_FLAGS.

[AnthonyLatsis]

To be clear, the options should be added via compile_options at the top level, not by changing the CMAKE_{C,CXX,Swift}_FLAGS.

Thank you!

[AnthonyLatsis]

@etcwilde ping

[AnthonyLatsis]

@swift-ci please smoke test

[AnthonyLatsis]

@swift-ci please smoke test macOS

[AnthonyLatsis]

@swift-ci please smoke test macOS