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.

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

Clean up dependency graph and imports #218

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Aug 13, 2021

Motivation:

AWSLambdaRuntimeCore and MockServer both have dependencies on the NIO
module, but they don't express this in their Package.swift: they
implicitly rely on these dependencies being met by the NIOHTTP1 import.
This was always brittle: it could lead to timing issues in the build
that could lead to it failing.

Modifications:

  • Correctly express the dependency on NIO in Package.swift
  • Correctly import _NIOConcurrency in files where it's used

Result:

Better and more accurate builds.

Motivation:

AWSLambdaRuntimeCore and MockServer both have dependencies on the NIO
module, but they don't express this in their Package.swift: they
implicitly rely on these dependencies being met by the NIOHTTP1 import.
This was always brittle: it could lead to timing issues in the build
that could lead to it failing.

Modifications:

- Correctly express the dependency on NIO in Package.swift
- Correctly import _NIOConcurrency in files where it's used

Result:

Better and more accurate builds.
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Thanks for spotting and fixing this!

@fabianfett fabianfett merged commit 48dc507 into swift-server:main Aug 13, 2021
@fabianfett
Copy link
Member

Should we cherry pick this into a patch release? @tomerd @Lukasa

@tomerd
Copy link
Contributor

tomerd commented Aug 13, 2021

Should we cherry pick this into a patch release? @tomerd @Lukasa

+1

@tomerd tomerd added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Aug 13, 2021
Lukasa added a commit to Lukasa/swift-nio that referenced this pull request Aug 16, 2021
Motivation:

In apple#1935 I removed NIO as a dependency of a number of our library
targets. This unfortunately led to some downstream breakage in modules
that were (incorrectly) assuming they could import NIO without
expressing a dependency on it in their Package.swift, e.g.
swift-server/swift-aws-lambda-runtime#218.

While a forums thread
(https://forums.swift.org/t/semantic-versioning-should-removing-a-dependency-be-a-semver-major/51179)
is ongoing to discuss the implications of this, we should re-add the
dependency to undo the breakage against main. I've validated this
locally: merely having the dependency is enough, we don't have to use
it.

Modifications:

- Re-add NIO as a dependency to:
    - _NIOConcurrency
    - NIOFoundationCompat
    - NIOHTTP1
    - NIOTLS
    - NIOWebSocket

Result:

Broken downstreams can build again.
Lukasa added a commit to apple/swift-nio that referenced this pull request Aug 16, 2021
Motivation:

In #1935 I removed NIO as a dependency of a number of our library
targets. This unfortunately led to some downstream breakage in modules
that were (incorrectly) assuming they could import NIO without
expressing a dependency on it in their Package.swift, e.g.
swift-server/swift-aws-lambda-runtime#218.

While a forums thread
(https://forums.swift.org/t/semantic-versioning-should-removing-a-dependency-be-a-semver-major/51179)
is ongoing to discuss the implications of this, we should re-add the
dependency to undo the breakage against main. I've validated this
locally: merely having the dependency is enough, we don't have to use
it.

Modifications:

- Re-add NIO as a dependency to:
    - _NIOConcurrency
    - NIOFoundationCompat
    - NIOHTTP1
    - NIOTLS
    - NIOWebSocket

Result:

Broken downstreams can build again.
fabianfett pushed a commit that referenced this pull request Aug 24, 2021
- Correctly express the dependency on NIO in Package.swift
- Correctly import _NIOConcurrency in files where it's used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants