Skip to content

Conversation

@adam-fowler
Copy link

@adam-fowler adam-fowler commented Jun 7, 2025

Description

Replaced platform requirement in Package.swift with @available checks. This allows package-benchmark to be included in other packages without them requiring the additional platform requirements of package-benchmark.

Unfortunately we still need a 10.15 requirement as HdrHistogram requires it.

How Has This Been Tested?

I included package-benchmark in another package not requiring macOS 13 and verified everything compiled fine.

Minimal checklist:

  • I have performed a self-review of my own code
  • I have added DocC code-level documentation for any public interfaces exported by the package
  • I have added unit and/or integration tests that prove my fix is effective or that my feature works

@hassila
Copy link
Contributor

hassila commented Jun 7, 2025

Thanks!

@dimlio wdyt of updating hdr histogram similarly too?

@hassila hassila changed the title Replace platform requirement with @available markup chore: Replace platform requirement with @available markup Jun 8, 2025
@hassila
Copy link
Contributor

hassila commented Jul 1, 2025

@adam-fowler - most consumers of Benchmark simply puts a subdirectory with a separate Package.swift for the benchmarks that runs with the required platforms - have a look at e.g. Foundation - was this something you considered?

@adam-fowler
Copy link
Author

@adam-fowler - most consumers of Benchmark simply puts a subdirectory with a separate Package.swift for the benchmarks that runs with the required platforms - have a look at e.g. Foundation - was this something you considered?

Yeah I do that with Hummingbird. But it is much nicer to be able to include it in the main package.swift as we can ensure we don't break the benchmarks with code changes.

@hassila
Copy link
Contributor

hassila commented Jul 1, 2025

Oh, we just run the benchmarks as one CI step that is required to build too, but maybe that is not feasible?

@MahdiBM
Copy link
Contributor

MahdiBM commented Jul 1, 2025

I think using subdirectories is the better way to go so a package is not pulling this package unnecessarily, but I also do think this PR is fine to merge since a lot of different first-party packages have started doing this (and for good reasons) so package-benchmark would be consistent with those, and also the fact that it's not the easiest to figure out how to properly have a sub-package so someone might still want to do that anyway from a UX perspective.

See https://github.com/MahdiBM/swift-dns/Benchmarks for example, where I had to do a bunch of symlinking and then manually put together the Package.swift of the benchmarking sub-module like this to avoid needing to expose some APIs as public.

@adam-fowler
Copy link
Author

I think using subdirectories is the better way to go so a package is not pulling this package unnecessarily

SwiftPM does not pull the unused Benchmark dependency. I've just verified that

@MahdiBM
Copy link
Contributor

MahdiBM commented Jul 1, 2025

@adam-fowler that's news to me. I just tried MultipartKit's Benchmarks sub-module and even if i do swift build --target MultipartKit it still does pull package-benchmark although that target has no dependency on package-benchmark.
How did you test to confirm it doesn't pull package-benchmark in your case? is that package public?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants