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.

Sign up for GitHub

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

Jump to bottom

Suppress redundant linkage warnings due to dependency linkage propagation #7404

Merged
merged 16 commits into from Mar 19, 2024
Merged

Suppress redundant linkage warnings due to dependency linkage propagation #7404

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
60ee80d
A fix for overriding environment variables rdar://118814900
pusukuri Nov 30, 2023
c6da47b
track SDK dependencies rdar://115777026
pusukuri Dec 8, 2023
ef5ce43
removed unnecessary variable
pusukuri Jan 17, 2024
1a9d270
removed unnecessary check
pusukuri Jan 18, 2024
20ae44e
Merge remote-tracking branch 'upstream/main'
pusukuri Jan 19, 2024
1169440
Merge remote-tracking branch 'upstream/main'
pusukuri Jan 20, 2024
a99e152
derive dynamic libraries for explicitly linking rdar://108561857
pusukuri Jan 22, 2024
53fbf70
improved the test case
pusukuri Jan 23, 2024
a1aa166
Merge branch 'apple:main' into main
pusukuri Jan 24, 2024
c8503f0
rdar://113256834 consider plugin build command input files into consi…
pusukuri Jan 26, 2024
407a003
fixing the test case
pusukuri Jan 30, 2024
1f7173d
fix code formatting
pusukuri Feb 5, 2024
3eca47a
Merge branch 'main' of github.com:pusukuri/swift-package-manager
pusukuri Feb 21, 2024
6f81983
rdar://112671586 Suppress redundant linkage warnings due to dependenc…
pusukuri Mar 15, 2024
f7e6759
fixing test cases
pusukuri Mar 19, 2024
8c16a1b
Apply suggestions from code review
MaxDesiatov Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions Sources/Build/BuildDescription/ProductBuildDescription.swift
View file
Open in desktop
Expand Up @@ -189,6 +189,12 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription

var isLinkingStaticStdlib = false
let triple = self.buildParameters.triple

// radar://112671586 supress unnecessary warnings
if triple.isMacOSX {
args += ["-Xlinker", "-no_warn_duplicate_libraries"]
}

switch derivedProductType {
case .macro:
throw InternalError("macro not supported") // should never be reached
Expand Down
5 changes: 5 additions & 0 deletions Sources/XCBuildSupport/PIFBuilder.swift
View file
Open in desktop
Expand Up @@ -672,6 +672,11 @@ final class PackagePIFProjectBuilder: PIFProjectBuilder {
impartedSettings[.OTHER_LDFLAGS, default: ["$(inherited)"]].append("-lc++")
}

// radar://112671586 supress unnecessary warnings
#if os(macOS)
Copy link
Member

Choose a reason for hiding this comment

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

I think the #if is redundant here because this is essentially dead code on non-macOS today.

impartedSettings[.OTHER_LDFLAGS, default: ["$(inherited)"]].append("-Wl,-no_warn_duplicate_libraries")
#endif

addSources(target.sources, to: pifTarget)

// Handle the target's dependencies (but don't link against them).
Expand Down
15 changes: 15 additions & 0 deletions Tests/BuildTests/BuildPlanTests.swift
View file
Open in desktop
Expand Up @@ -773,6 +773,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -1186,6 +1187,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-dead_strip",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
Expand Down Expand Up @@ -1276,6 +1278,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -1435,6 +1438,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -1625,6 +1629,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -1779,6 +1784,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -2027,6 +2033,7 @@ final class BuildPlanTests: XCTestCase {
buildPath.appending(components: "PkgPackageTests.xctest", "Contents", "MacOS", "PkgPackageTests")
.pathString,
"-module-name", "PkgPackageTests",
"-Xlinker", "-no_warn_duplicate_libraries",
"-Xlinker", "-bundle",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path/../../../",
"@\(buildPath.appending(components: "PkgPackageTests.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -2125,6 +2132,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-dead_strip",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
Expand Down Expand Up @@ -2489,6 +2497,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -2636,6 +2645,7 @@ final class BuildPlanTests: XCTestCase {
"-o", buildPath.appending(components: "Foo").pathString,
"-module-name", "Foo",
"-lBar-Baz",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "Foo.product", "Objects.LinkFileList"))",
Expand All @@ -2651,6 +2661,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "libBar-Baz.dylib").pathString,
"-module-name", "Bar_Baz",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-library",
"-Xlinker", "-install_name", "-Xlinker", "@rpath/libBar-Baz.dylib",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
Expand Down Expand Up @@ -2803,6 +2814,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "liblib.dylib").pathString,
"-module-name", "lib",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-library",
"-Xlinker", "-install_name", "-Xlinker", "@rpath/liblib.dylib",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
Expand Down Expand Up @@ -2942,6 +2954,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "liblib.dylib").pathString,
"-module-name", "lib",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-library",
"-Xlinker", "-install_name", "-Xlinker", "@rpath/liblib.dylib",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
Expand All @@ -2956,6 +2969,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"-Xlinker", "-rpath", "-Xlinker", "@loader_path",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
Expand Down Expand Up @@ -6158,6 +6172,7 @@ final class BuildPlanTests: XCTestCase {
"-L", buildPath.pathString,
"-o", buildPath.appending(components: "exe").pathString,
"-module-name", "exe",
"-Xlinker", "-no_warn_duplicate_libraries",
"-emit-executable",
"@\(buildPath.appending(components: "exe.product", "Objects.LinkFileList"))",
"-Xlinker", "-rpath", "-Xlinker", "/fake/path/lib/swift-5.5/macosx",
Expand Down
2 changes: 1 addition & 1 deletion Tests/BuildTests/BuildSystemDelegateTests.swift
View file
Open in desktop
Expand Up @@ -25,7 +25,7 @@ final class BuildSystemDelegateTests: XCTestCase {
try XCTSkipIf(true, "test is only supported on macOS")
#endif
let (fullLog, _) = try executeSwiftBuild(fixturePath)
XCTAssertTrue(fullLog.contains("ld: warning: ignoring duplicate libraries: '-lz'"), "log didn't contain expected linker diagnostics")
XCTAssertFalse(fullLog.contains("ld: warning: ignoring duplicate libraries: '-lz'"), "log contains unnecessary linker warnings")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's correct to change this test, it was supposed to ensure that linker warnings are properly propagated. We'll need to find a different linker warning that we can check for here or remove the test if appropriate.

Copy link
Contributor Author

@pusukuri pusukuri Mar 15, 2024 •
edited

Choose a reason for hiding this comment

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

Thank you @neonichu I will talk with @MaxDesiatov and finalize it.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Boris, with this PR as it is we're losing test coverage here. This test needs to change to check for a different warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked with @MaxDesiatov and made changes appropriately for the above test case.

}
}

Expand Down
6 changes: 4 additions & 2 deletions Tests/XCBuildSupportTests/PIFBuilderTests.swift
View file
Open in desktop
Expand Up @@ -1312,6 +1312,7 @@ class PIFBuilderTests: XCTestCase {
"-fmodule-map-file=$(OBJROOT)/GeneratedModuleMaps/$(PLATFORM_NAME)/FooLib1.modulemap"
])
XCTAssertEqual(settings[.OTHER_LDRFLAGS], [])
XCTAssertEqual(settings[.OTHER_LDFLAGS], ["$(inherited)", "-Wl,-no_warn_duplicate_libraries"])
}
}

Expand Down Expand Up @@ -1384,7 +1385,7 @@ class PIFBuilderTests: XCTestCase {
"-fmodule-map-file=$(OBJROOT)/GeneratedModuleMaps/$(PLATFORM_NAME)/FooLib2.modulemap"
])
XCTAssertEqual(settings[.OTHER_LDRFLAGS], [])
XCTAssertEqual(settings[.OTHER_LDFLAGS], ["$(inherited)", "-lc++"])
XCTAssertEqual(settings[.OTHER_LDFLAGS], ["$(inherited)", "-lc++", "-Wl,-no_warn_duplicate_libraries"])
XCTAssertEqual(settings[.OTHER_SWIFT_FLAGS], [
"$(inherited)",
"-Xcc", "-fmodule-map-file=$(OBJROOT)/GeneratedModuleMaps/$(PLATFORM_NAME)/FooLib2.modulemap"
Expand Down Expand Up @@ -1463,6 +1464,7 @@ class PIFBuilderTests: XCTestCase {
"-fmodule-map-file=$(OBJROOT)/GeneratedModuleMaps/$(PLATFORM_NAME)/BarLib.modulemap"
])
XCTAssertEqual(settings[.OTHER_LDRFLAGS], [])
XCTAssertEqual(settings[.OTHER_LDFLAGS], ["$(inherited)", "-Wl,-no_warn_duplicate_libraries"])
XCTAssertEqual(settings[.OTHER_SWIFT_FLAGS], [
"$(inherited)",
"-Xcc", "-fmodule-map-file=$(OBJROOT)/GeneratedModuleMaps/$(PLATFORM_NAME)/BarLib.modulemap"
Expand Down Expand Up @@ -2393,7 +2395,7 @@ class PIFBuilderTests: XCTestCase {
target.checkImpartedBuildSettings { settings in
XCTAssertEqual(settings[.GCC_PREPROCESSOR_DEFINITIONS], nil)
XCTAssertEqual(settings[.HEADER_SEARCH_PATHS], nil)
XCTAssertEqual(settings[.OTHER_LDFLAGS], ["$(inherited)", "-lz"])
XCTAssertEqual(settings[.OTHER_LDFLAGS], ["$(inherited)", "-Wl,-no_warn_duplicate_libraries", "-lz"])
XCTAssertEqual(settings[.OTHER_SWIFT_FLAGS], nil)
}
}
Expand Down