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

Hide more llbuild-specific APIs #7387

Merged
merged 4 commits into from Mar 4, 2024
Merged

Hide more llbuild-specific APIs #7387

merged 4 commits into from Mar 4, 2024

Conversation

MaxDesiatov
Copy link
Member

@MaxDesiatov MaxDesiatov commented Mar 3, 2024

These APIs should not be marked public to prevent SwiftPM clients from adopting them by accident.

This APIs should not be marked `public` to prevent SwiftPM clients from adopting those by accident.
@MaxDesiatov MaxDesiatov added build system Changes to interactions with build systems public API Changes to the public API of SwiftPM labels Mar 3, 2024
@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov MaxDesiatov self-assigned this Mar 3, 2024
@MaxDesiatov
Copy link
Member Author

@swift-ci test

@rauhul
Copy link
Contributor

rauhul commented Mar 3, 2024

I'd just like to say thanks for going through the work of minifying the SwiftPM api surface, this makes it much easier to make changes without breaking clients

@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov MaxDesiatov enabled auto-merge (squash) March 4, 2024 09:40
@MaxDesiatov
Copy link
Member Author

@swift-ci test windows

@MaxDesiatov MaxDesiatov merged commit 5185c03 into main Mar 4, 2024
5 checks passed
@MaxDesiatov MaxDesiatov deleted the maxd/hide-build-apis branch March 4, 2024 16:54
@krzyzanowskim
Copy link
Contributor

@MaxDesiatov What is the replacement API to make builds now? I use BuildOperation just like SwiftBuildCommand does.

Now, only SPM can use that API and that doesn't seem fair to me. what am I missing here?

@MaxDesiatov
Copy link
Member Author

There is no replacement API outside of PackagePlugin, this wasn't supposed to be an API in the first place and was exposed because package access control was unavailable at the time.

Build system internals is an unversioned implementation detail, which can and will be changed without notice. Removing public from these symbols allows us to evolve it and migrate to structured concurrency or overhaul how it works altogether without constraints of potentially breaking external adopters.

Keeping them public effectively freezes them and require us to maintain that API shape indefinitely, which is not something that can be provided at the moment.

@krzyzanowskim
Copy link
Contributor

Keeping them public effectively freezes them and require us to maintain that API shape indefinitely, which is not something that can be provided at the moment.

that is not true. It was never "freezed" by any means. It is actively used by the CLI client and could be used by third-party clients. Now that is no longer possible 😢

@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented Apr 4, 2024

SwiftPM CLI client is maintained in this same repository and can be changed in lockstep in the same PR. We would be notified by CI job failures and could fix it immediately if anything breaks on that front. SourceKit-LSP breakages are also exposed by SwiftPM CI jobs in PR testing and must be resolved before a PR is merged.

This is not the case for external clients. There's no compatibility suite for those and no versioning of what became a public API by accident or because the language didn't allow us to keep it private at the time when it was developed.

@krzyzanowskim
Copy link
Contributor

SwiftPM CLI client is maintained in this same repository and can be changed in lockstep in the same PR.

it is as it should be. I don't see a clear reason to shut down the API completly for other clients. On the contrary, if API is good enough for CLI, it's good enough for third-party clients. Since API is not stable (which is ok) it's ok to change it without providing backward compatibility. I don't find that a trouble nor unusual - I understand how it is convenient for the SwiftPM project needs itself, but the value in keep the API open (the way I see it) can be only beneficial for SwiftPM adoption outside Apple.

There's no compatibility suite for those and no versioning of what became a public API by accident or because the language didn't allow us to keep it private at that time.

Maybe swift SPI mechanism is better suited for that after all.

@MaxDesiatov
Copy link
Member Author

MaxDesiatov commented Apr 4, 2024

I don't see a clear reason to shut down the API completly for other clients.

I think I've provided enough reasons in this PR and the linked issue, I don't see a point in going in circles and rehashing those again.

if API is good enough for CLI, it's good enough for third-party clients

I don't see how this conjecture is made. Needs, requirements, and expectations of CLI maintained in the same package as other SwiftPM internals, as opposed to third-party clients, are significantly different. An internal SPI good enough for CLI maintained in the same package is almost always not good enough for third-party clients, as the latter require stability, versioning, and a commitment of continued maintenance of those public APIs.

Since API is not stable (which is ok) it's ok to change it without providing backward compatibility.

Which is what happened in this PR: an unstable API (that was not meant to be public in the first place) was changed and no backward compatibility is provided.

Maybe swift SPI mechanism is better suited for that after all.

This specific point in isolation is correct. I'm considering what still remains public outside of PackageDescription and PackagePlugin modules and can't be made package to be hidden with @_spi(SwiftPMInternal) to additionally indicate that symbols outside of those two modules should not be adopted and relied on by 3rd-party clients.

@krzyzanowskim
Copy link
Contributor

An internal SPI good enough for CLI maintained in the same package is almost always not good enough for third-party clients, as the latter require stability, versioning, and a commitment of continued maintenance of those public APIs.

disagree. sourcekit-lsp relies on that unstable API, and stability is not a requirement. that's my whole point. API does not have to be stable unless declared as such, which then sets an expectation. I don't think the remaining public API is stable, too anyway.

@MaxDesiatov
Copy link
Member Author

I'll reiterate again that SourceKit-LSP case is different as it is always built in lockstep with SwiftPM. For it API stability is not a requirement because it can't be broken for it, we literally can't merge a SwiftPM PR that breaks SourceKit-LSP, required CI jobs won't pass in that case.

This is not the case for other clients.

@krzyzanowskim
Copy link
Contributor

This is not the case for other clients.

I'll reiterate again that this is an artificial requirement you put on that. As an "other clients" representative, I can assure you that's not an issue for me, and I accept the consequences of an unstable API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system Changes to interactions with build systems public API Changes to the public API of SwiftPM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants