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

If FoundationEssentials is available use it #158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adam-fowler
Copy link

Use FoundationEssentials if it is available

Motivation:

Reduce size of any application using swift-metrics on linux

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

As noted in other repos, this is a semver major change, so we’ll need to work out how to stage this in.

@adam-fowler
Copy link
Author

adam-fowler commented Jan 13, 2025

As noted in other repos, this is a semver major change, so we’ll need to work out how to stage this in.

Is this a breaking change?

It was a breaking change in swift-nio-transport-services because the import NIOFoundationCompat was removed, but removing an import Foundation doesn't make Foundation unavailable as an import in packages dependent on swift-metrics.

@Lukasa
Copy link
Contributor

Lukasa commented Jan 13, 2025

Yes, it's a breaking change. Reproducing my comment from swift-crypto:

It doesn’t make the full API available, but it makes some things available. An example of one such problem is available at https://forums.swift.org/t/pitch-fixing-member-import-visibility/71432.

The Swift team consider this a bug, and I agree, but our semver policies take the language as it is, not as it is intended to be. So this will require us to burn a major.

@adam-fowler
Copy link
Author

pfff

@t089
Copy link

t089 commented Jan 13, 2025

Yeah :( I wonder if we could get away with it, though. Today, most apps (knowingly or unknowingly) anyhow somehow import Foundation somewhere, so unless you are actively trying to avoid it, you are anyhow already importing it. And if you are actively trying to avoid it, you do not use API from it, so this change will not be source breaking.

@Lukasa
Copy link
Contributor

Lukasa commented Jan 13, 2025

And if you are actively trying to avoid, you do not use API from it, so this change will not be source breaking.

There's a certain "if a tree falls in the forest and no-one is around to hear it" quality to this argument. It's true that whether a change is source-breaking is a function of whether anyone is actually using the API. The wrinkle is that it's very hard for us to validate that they aren't. My bias is strongly against taking that risk, usually.

In the case of metrics in particular, there are good reasons to think that we may want to modernise the API. We could do these two things at once, put the more modern API in a new module and deprecate the old interface. This will cause a natural community migration away from the deprecated API and toward a new one, while avoiding a source break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants