[PackageLoading] Filter out library creation note from LINK

[stevapple]

Filter out library creation note from LINK in ManifestLoader.

Motivation:

We're using LINK for linking on Windows by default, and it will always emit a note like this:

   Creating library TMP_DIR\package-manifest.lib and object TMP_DIR\package-manifest.exp

Since there's no way to opt out, and this isn't a real warning, we should filter it out in ManifestLoader.

Modifications:

• Downgrade library creation note from LINK in ManifestLoader to debug level.
• Remove rdar://73710910 workaround.

Result:

Users no longer see the false warning of Creating library ... and object ... when building the manifest.

[stevapple]

This is caused by Swift Driver no longer defaulting to -fuse-ld=lld, which is new in Swift 5.7.

[tomerd]

is there another way to deal with this issue? having SwiftPM filter it is a bit of tech debt

[tomerd]

cc @artemcm

[stevapple]

is there another way to deal with this issue? having SwiftPM filter it is a bit of tech debt

Or we can force using lld on manifests, more elegant probably?

[neonichu]

Or we can force using lld on manifests, more elegant probably?

We could add an #else (or flip the if-condition) here with Windows specific flags: https://github.com/apple/swift-package-manager/blob/main/Sources/PackageLoading/ManifestLoader.swift#L720-L724

[stevapple]

better now? @neonichu @tomerd

[tomerd]

looks better to me, but @compnerd to approve as Windows platform release manager

[compnerd]

I really would rather retain the ability to use link - there are differences in the linker that has been helpful to compare link vs lld. The C++ never defaulted to lld either, and that is again by design. We do control the flags by means of the platform providing default flags from the Info.plist. I think that "forcing lld" by means honoring that setting is preferable, and the fact that we don't honor could be argued as being unexpected for the user. (Simply applying those settings will implicitly switch to lld as we do already use lld for the rest of the build)

It would be good to have a way to ignore stdout output from the tools though. The toolchain tools are permitted to write to the console, SPM should be able to deal with that.

[neonichu]

It would be good to have a way to ignore stdout output from the tools though. The toolchain tools are permitted to write to the console, SPM should be able to deal with that.

I don't think it's correct to just ignore output, there could be actual compiler warnings there which are relevant. Today, we're treating any output as warnings here: https://github.com/apple/swift-package-manager/blob/main/Sources/PackageLoading/ManifestLoader.swift#L492 which maybe is a bit to simple?

[compnerd]

It would be good to have a way to ignore stdout output from the tools though. The toolchain tools are permitted to write to the console, SPM should be able to deal with that.

I don't think it's correct to just ignore output, there could be actual compiler warnings there which are relevant. Today, we're treating any output as warnings here: https://github.com/apple/swift-package-manager/blob/main/Sources/PackageLoading/ManifestLoader.swift#L492 which maybe is a bit to simple?

Many tools, particularly on Windows, would print a logo "message", which does make sense to ignore. Ignoring the error stream would be bad, I agree. I think that using lld by means of the platform flags being injected appropriately (i.e. the user can still use -Xmanifest to override it) is fine, but, that is not really solving the issue, just hiding it in some cases. But, there are other reasons why injecting the flags is a good thing, namely the behaviour is identical across the manifest build and the regular build - so we should do that, but I don't think that it really resolves the issue.

[stevapple]

But, there are other reasons why injecting the flags is a good thing, namely the behaviour is identical across the manifest build and the regular build - so we should do that, but I don't think that it really resolves the issue.

I think at least the specific problem is resolved, because we're only regarding output as warnings for manifest builds. When building the packages these outputs are just noises that we can decide later about whether and how to filter them.

[stevapple]

WDYT @tomerd @neonichu @compnerd

[compnerd]

The fundamental problem here is that SWIFTC_FLAGS aren't being wired in, that is the proper thing to do here rather than hardcoding the linker.

[tomerd]

The fundamental problem here is that SWIFTC_FLAGS aren't being wired in, that is the proper thing to do here rather than hardcoding the linker.

that would make things much more palatable. @stevapple do you need any hints from @compnerd on what is going on?

[tomerd]

@stevapple @compnerd what is next with this one? should we close this PR?

[stevapple]

As requested, the only left piece of work is to respect the linker set by environment SWIFTC_FLAGS? If it is, I think I have some time now to get it quickly fixed🤔

In the long run, output filtering might be the right direction. Hopefully the new RegEx feature should help a lot there.

[stevapple]

I added a filter per @compnerd’s request, so this shouldn’t override -Xmanifest and SWIFTC_FLAGS. I also agreed on his conclusion that output filtering (or downgrading) would be better in the long run, but that holds on pending discussion, so this should be the solution for now.

[compnerd]

This still does not do the right thing - honoring the flags. This is hardcoding lld, which is undesirable.

[stevapple]

This still does not do the right thing - honoring the flags. This is hardcoding lld, which is undesirable.

I believe you’re misunderstanding the case here. SWIFTC_FLAGS are already injected in https://github.com/apple/swift-package-manager/blob/99df615d901ac523cdd8352fe57cbfa5756017a1/Sources/PackageLoading/ManifestLoader.swift#L542 and -Xmanifest in https://github.com/apple/swift-package-manager/blob/99df615d901ac523cdd8352fe57cbfa5756017a1/Sources/PackageLoading/ManifestLoader.swift#L566.
Hardcoding lld is the answer because we need it as a default, or a user may run into false warning with a plain swift build.

[stevapple]

cc @compnerd, we finally returned to the output filtering path.

[compnerd]

Does this not drop the existing filtering for the rdar workaround? I would rather that we kept that given the intent of the change here.

Why the additional change for recording the compiledManifestFile to just filter the message?

[stevapple]

Why the additional change for recording the compiledManifestFile to just filter the message?

I don't want to hard-code the naming rule of compiled manifest (it's \(packageIdentity)-manifest now, but we should preserve it as an implementation detail, which is subject to change at any time). So the best solution is to get it from the path.

[stevapple]

Does this not drop the existing filtering for the rdar workaround? I would rather that we kept that given the intent of the change here.

That's a piece of legacy which is useless and can't fit nicely IMO. Maybe we can first remove the workaround in a separate PR?

[stevapple]

Maybe we can first remove the workaround in a separate PR?

Cherry-picked to #5760

[stevapple]

I bet this is ready for test and merge?

[tomerd]

@compnerd @stevapple is there a cleaner way to deal with that does not parse the output? I can see how this would work, but it feels bolted on, rather than a systematic solution

[compnerd]

Thanks for elaborating on the FIXME

[compnerd]

My understanding is that @stevapple's concern is the additional information that is emitted by the (desired) linker. Since there is no way to control that specific message, if we are looking to suppress that message then filtering based on a match seems reasonable.

The problem here is that we are trying to invoke tools and process their output based on where the output goes. Although this particular message is presented on stderr, it is not an error nor a warning but rather an informational message. The other option is to consider doing the inverse of this: filtering the acceptance rather than the rejection.

From https://learn.microsoft.com/en-us/cpp/build/reference/link-output?view=msvc-170:

LINK issues error and warning messages in the form LNKnnnn. This error prefix and range of numbers are also used by LIB, DUMPBIN, and EDITBIN.

We could filter the output explicit for warnings and errors and suppress everything else. That should also accommodate the raised concern I believe.

[stevapple]

We could filter the output explicit for warnings and errors and suppress everything else. That should also accommodate the raised concern I believe.

Just as I mentioned in FIXME, the tricky point here is that we get all the outputs (whether from stdout or stderr, from compiler or linker) messed up in a single String — so even if we know the linker is link, we cannot prove all the outputs come from it. That’s why I didn’t take such path, and instead chose to only filter a single line with desired format. That’s the only determined output we can have, and all additional lines are beyond control.

[neonichu]

I think which ever way is being taken, it doesn't seem right as an ad-hoc thing in manifest loader. We should instead know which type of tools we are invoking and could have a filter based on that which is used by the manifest loader.

[stevapple]

We should instead know which type of tools we are invoking and could have a filter based on that which is used by the manifest loader.

IIRC this is a bit out of control — we’re just invoking swiftc here, and we don’t know which tools Swift Driver will invoke. The output can’t tell either.

[compnerd]

Ah, that is a good point, I think that the chain might actually be: swift-build -> swiftc -> clang -> link. We would have lost the provenance along the way.

[neonichu]

I think if we want to filter, we need to take tighter control of what is happening. So we could split up the compilation process of manifest loading into calling swiftc to produce a .o and then calling the linker to produce an executable from that.

BTW, note that the same process also happens for plugin compilation, so if we just have the filtering at the manifest loading level, we would still get the same type of issue when building a plugin.

[stevapple]

I’m afraid that over-complicates the process. It also unnecessarily duplicates logic between Swift Driver and SwiftPM.

BTW, note that the same process also happens for plugin compilation, so if we just have the filtering at the manifest loading level, we would still get the same type of issue when building a plugin.

It seems plugin runner is treating compiler output as info instead of warning, so the noise affects less.🤔