Unify the two versions of PackageDescription into a single one using availability annotations instead of compile conditionals #3464

abertelrud · 2021-04-30T20:10:04Z

Replace the compile-time conditionals in PackageDescription with availability annotations, and build a single runtime library that can be used for all package tools versions.

Motivation:

This is preparatory work for #3431 and will also make it easier for clients of libSwiftPM, since only one version of PackageDescription needs to be built.

Background:

The runtime support library for PackageDescription is currently built twice: once for SwiftPM 4.0 and another time for 4.2 and later. This is done using compile-time conditionals, producing two separate binaries in the file system. Code in the manifest loader chooses between them based on the declared tools version of the package.

Having two versions of the library has several problems:

it makes the build scripts more complicated; this is true for the bootstrap script, the CMake files, and the XBS build script
it makes the runtime code more complicated, and because PackageDescription 4 isn’t encountered as much, errors can go undetected
because SwiftPM doesn’t support building multiple version of the same target, only 4.2 and later is available in debug builds

We can unify the two versions using @Availability annotations, so that we only need one PackageDescription library. We should still keep it in a separate subdirectory from the PackagePlugins runtime support library. Instead of using the names “4” and “4_2” as the subdirectory names, we should take this opportunity to separate them based on something more meaningful, such as “ManifestAPI” and “PluginAPI”.

Modifications:

replace compile-time conditionals with availability annotations
adjust the package manifest to pass -package-description-version 5.5 and -enable-library-evolution
adjust a unit test to account for a different error message now when using 4.2 and trying to use ints for SwiftLanguageVersion

Result:

This builds PackageDescription libraries that aren't dependent on the compile time condition.

rdar://77441062

abertelrud · 2021-04-30T20:11:58Z

Tests/PackageLoadingTests/PD4_2LoadingTests.swift

@@ -98,7 +98,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
            guard case let ManifestParseError.invalidManifestFormat(output, _) = error else {
                return XCTFail()
            }
-            XCTAssertMatch(output, .and(.or(.contains("expected argument type"), .contains("expected element type")), .contains("SwiftVersion")))
+            XCTAssertMatch(output, .and(.contains("'init(name:pkgConfig:providers:products:dependencies:targets:swiftLanguageVersions:cLanguageStandard:cxxLanguageStandard:)' is unavailable"), .contains("was obsoleted in PackageDescription 4.2")))


This accounts for the difference in error message. The "is unavailable" is arguably a more accurate one anyway.

abertelrud · 2021-04-30T20:12:30Z

@swift-ci please smoke test

abertelrud · 2021-05-02T05:13:27Z

@swift-ci please smoke test

Package.swift

abertelrud · 2021-05-03T01:04:36Z

@swift-ci please smoke test

abertelrud · 2021-05-03T01:29:12Z

Sources/PackageDescription/CMakeLists.txt

-        Target.swift
-        Version.swift
-        Version+StringLiteralConvertible.swift)
+add_library(PackageDescription


While unwinding the loop I also made this conform to the 2-space indentation that we seem to be using for CMakefiles.

abertelrud · 2021-05-03T01:30:47Z

Utilities/bootstrap

@@ -397,22 +397,21 @@ def install_swiftpm(prefix, args):
    runtime_lib_dest = os.path.join(prefix, "lib", "swift", "pm")
    runtime_lib_src = os.path.join(args.bootstrap_dir, "pm")


Note that for now we’re still installing the bootstrap-built PackageDescription. This will change in the follow-on PR.

abertelrud · 2021-05-03T04:14:43Z

@swift-ci please smoke test

abertelrud · 2021-05-03T06:06:36Z

Sources/PackagePlugin/CMakeLists.txt

@@ -7,60 +7,63 @@
 # See http://swift.org/CONTRIBUTORS.txt for Swift project authors

 add_library(PackagePlugin
-    PublicAPI/CommandConstructor.swift


While modifying the CMakefile I made it conform to the 2-space indentation that we seem to be using for CMakefiles.

abertelrud · 2021-05-03T06:08:08Z

Sources/PackageDescription/CMakeLists.txt

-      DESTINATION lib/swift/pm/${PACKAGE_DESCRIPTION_VERSION})
-endforeach()
+install(TARGETS PackageDescription
+  DESTINATION lib/swift/pm/ManifestAPI)


Rather than relying on a directory named after the version number to keep the library apart from the PackagePlugin library, I added a subdirectory here named after the subsystem for which this is an API. The plugin one is called PluginAPI, and I can imagine that we'd have additional ones as needed.

abertelrud · 2021-05-03T06:09:04Z

Sources/PackagePlugin/CMakeLists.txt

 endif()

 install(TARGETS PackagePlugin
-    DESTINATION lib/swift/pm)
+  DESTINATION lib/swift/pm/PluginAPI)


With PackageDescription no longer living in a directory named after the version number to keep the library apart from the PackagePlugin library, I put each subsystem's API into a separate subdirectory here named after the subsystem for which this is an API. The plugin one is called PluginAPI, and I can imagine that we'd have additional ones as needed.

tomerd

lgtm

would like to see what @neonichu thinks before we merge

…availability annotations instead of #if Replace the compile-time conditionals in PackageDescription with availability annotations. This lets us build a single PackageDescription, which will make the build scripts easier (with the goal of being able to build it entirely from the package itself). As part of this we also clean up the runtime library subdirectories. Until now we've had PackagePlugin directly in the runtime library directory, and PackageDescription under subdirectories called "4" and "4_2". This has sufficed in keeping PackageDescription and PackagePlugin in separate directories (so that a manifest or plugin script can't import both). This needs to be changed now that there is only one version of a PackageDescription, and anyway it's time to clean this up. So as part of this we also establish two subdirectories for the runtime APIs: "ManifestAPI" and "PluginAPI". If we add more kinds of scripts, we can then add new subdirectories here. rdar://77441062

abertelrud · 2021-05-03T18:13:02Z

@swift-ci please smoke test

abertelrud · 2021-05-03T20:59:46Z

Error doesn't look related. Rerunning.

abertelrud · 2021-05-03T20:59:53Z

@swift-ci please smoke test linux

abertelrud · 2021-05-03T22:35:14Z

@neonichu Do you have any concerns about this change?

aciidgh · 2021-05-07T02:04:58Z

Sources/PackageDescription/PackageDescription.swift

@@ -203,27 +189,21 @@ public final class Package {
    /// The list of package dependencies.
    public var dependencies: [Dependency]

-  #if PACKAGE_DESCRIPTION_4
-    /// The list of Swift versions that this package is compatible with.
-    public var swiftLanguageVersions: [Int]?


This is a breaking change, right? In the past, we went through the trouble creating two versions because we didn't want to break this part of the API but I doubt that there are a lot of packages that will actually get affected by this..

This updates the devtools packaging to account for the unified module layout that SPM adopted in swiftlang/swift-package-manager#3464.

abertelrud requested review from friedbunny, neonichu and tomerd as code owners April 30, 2021 20:10

abertelrud commented Apr 30, 2021

View reviewed changes

This was referenced Apr 30, 2021

Bootstrap script needs to build PackageDescription and PackagePlugin universal #3431

Merged

Android: add cross-compilation configuration file flag #3403

Merged

abertelrud marked this pull request as draft April 30, 2021 20:18

abertelrud self-assigned this Apr 30, 2021

abertelrud force-pushed the eng/unified-packagedescription branch from e1adf20 to 7fde8bc Compare May 2, 2021 04:55

abertelrud changed the title ~~WIP: Use availability annotations instead of #if to unify PackageDescriptiion 4.0 and 4.2+~~ WIP: Unify the two versions of PackageDescription into a single one using availability annotations instead of compile conditionals May 2, 2021

abertelrud force-pushed the eng/unified-packagedescription branch from 7fde8bc to 3b4a6ce Compare May 2, 2021 05:10

abertelrud force-pushed the eng/unified-packagedescription branch from 3b4a6ce to 7c78ff9 Compare May 3, 2021 00:38

abertelrud commented May 3, 2021

View reviewed changes

Package.swift Show resolved Hide resolved

abertelrud marked this pull request as ready for review May 3, 2021 01:27

abertelrud commented May 3, 2021

View reviewed changes

abertelrud force-pushed the eng/unified-packagedescription branch from 7c78ff9 to f157789 Compare May 3, 2021 04:12

abertelrud commented May 3, 2021

View reviewed changes

tomerd approved these changes May 3, 2021

View reviewed changes

abertelrud force-pushed the eng/unified-packagedescription branch from f157789 to 2d64d68 Compare May 3, 2021 18:12

neonichu approved these changes May 6, 2021

View reviewed changes

abertelrud merged commit 2d2b387 into swiftlang:main May 6, 2021

abertelrud deleted the eng/unified-packagedescription branch May 6, 2021 22:05

aciidgh reviewed May 7, 2021

View reviewed changes

compnerd mentioned this pull request May 7, 2021

packaging: update rules for unified package manifest in SPM compnerd/swift-build#363

Merged

abertelrud mentioned this pull request May 12, 2021

[5.5] Unify the two versions of PackageDescription into a single one using availability annotations instead of compile conditionals #3492

Merged

abertelrud mentioned this pull request Jun 16, 2021

Make the lookup of the PackageDescription dylib fall back on the versioned location if needed #3560

Merged

finagolfin mentioned this pull request Jun 15, 2021

[SR-14521] wrong RUNPATH in libPackagePlugin.so library in recent nightly toolchains #4433

Closed

		@@ -397,22 +397,21 @@ def install_swiftpm(prefix, args):
		runtime_lib_dest = os.path.join(prefix, "lib", "swift", "pm")
		runtime_lib_src = os.path.join(args.bootstrap_dir, "pm")

Unify the two versions of PackageDescription into a single one using availability annotations instead of compile conditionals #3464

Unify the two versions of PackageDescription into a single one using availability annotations instead of compile conditionals #3464

Uh oh!

Conversation

abertelrud commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Background:

Modifications:

Result:

Uh oh!

abertelrud Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Apr 30, 2021

Uh oh!

abertelrud commented May 2, 2021

Uh oh!

Uh oh!

abertelrud commented May 3, 2021

Uh oh!

abertelrud May 3, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud May 3, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud commented May 3, 2021

Uh oh!

abertelrud May 3, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud May 3, 2021

Choose a reason for hiding this comment

Uh oh!

abertelrud May 3, 2021

Choose a reason for hiding this comment

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud commented May 3, 2021

Uh oh!

abertelrud commented May 3, 2021

Uh oh!

abertelrud commented May 3, 2021

Uh oh!

abertelrud commented May 3, 2021

Uh oh!

aciidgh May 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abertelrud commented Apr 30, 2021 •

edited

Loading