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

Add API validation script to CI #1026

Closed
Lukasa opened this issue Jul 10, 2020 · 10 comments · Fixed by #1545
Closed

Add API validation script to CI #1026

Lukasa opened this issue Jul 10, 2020 · 10 comments · Fixed by #1545

Comments

@Lukasa
Copy link
Contributor

Lukasa commented Jul 10, 2020

SwiftNIO uses a script built on top of the Swift API digester command line tool to validate that PRs do not cause semver major breaking changes. This script could be adapted for the use cases of protobuf as well, and help avoid ambiguity around whether a change is breaking or not.

Note that, while this script is helpful, it does encounter some false positives. In particular it tends to believe that adding a protocol customisation point with a default implementation is a semver major, when so far as we can tell it is not (it's ABI breaking, I think, but not a semver major).

@thomasvl
Copy link
Collaborator

Happened to find https://github.com/ltetzlaff/swift-api-diff today. Don't suppose SwiftNIO is looking at make a standard github action out of this?

@thomasvl
Copy link
Collaborator

The script also needs jq installed, which isn't on macOS be default.

@Lukasa
Copy link
Contributor Author

Lukasa commented Jul 27, 2020

If the community has already done a standard GitHub action I'm more than happy to defer to that!

@thomasvl
Copy link
Collaborator

Since it seems to have a copy of the script, i wasn't sure how insync it is staying, etc. Is there any coordination with them?

@thomasvl
Copy link
Collaborator

It also looks like it posts back to the comments, which means it needs a github token. That likely means someone on Apple's side would need to sign off on it before we could use it here.

@tbkka I'm guessing the need for a secret to post back means it shouldn't be used here?

@thomasvl
Copy link
Collaborator

thomasvl commented Jul 27, 2020

Also looks like despite the script having xcrun support, the Xcode toolchains don't include swift-api-digester, so it also requires a custom toolchain install for macOS.

(I was looking to wire up a Makefile rule for running it, but it would be linux only.)

@tbkka
Copy link
Contributor

tbkka commented Jul 27, 2020

@thomasvl -- Yeah, we should avoid anything that needs a secret to post back.

@thomasvl
Copy link
Collaborator

@Lukasa should I open a nio issue asking for a standard action that doesn't require write access?

@Lukasa
Copy link
Contributor Author

Lukasa commented Jul 28, 2020

@thomasvl Sure, worst-case we can say we think it's out of scope. But I think an action is a nice approach.

@thomasvl
Copy link
Collaborator

Opened apple/swift-nio#1600

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Aug 16, 2022
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 12, 2024
Just run this on each PR to flag any changes. Since `main` is working toward a
2.0 release, we should be ok making breaking changes, but it likely doesn't hurt
to realize what the breaks are.

Fixes apple#1026
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 12, 2024
Just run this on each PR to flag any changes. Since `main` is working toward a
2.0 release, we should be ok making breaking changes, but it likely doesn't hurt
to realize what the breaks are.

Used some of the https://github.com/vapor projects for reference and the git
magic needed to for the action to run.

Fixes apple#1026
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 12, 2024
Just run this on each PR to flag any changes. Since `main` is working toward a
2.0 release, we should be ok making breaking changes, but it likely doesn't hurt
to realize what the breaks are.

Used some of the https://github.com/vapor projects for reference and the git
magic needed to for the action to run.

Fixes apple#1026
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 12, 2024
Just run this on each PR to flag any changes. Since `main` is working toward a
2.0 release, we should be ok making breaking changes, but it likely doesn't hurt
to realize what the breaks are.

Used some of the https://github.com/vapor projects for reference and the git
magic needed to for the action to run.

Fixes apple#1026
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 12, 2024
Just run this on each PR to flag any changes. Since `main` is working toward a
2.0 release, we should be ok making breaking changes, but it likely doesn't hurt
to realize what the breaks are.

Used some of the https://github.com/vapor projects for reference and the git
magic needed to for the action to run.

Fixes apple#1026
thomasvl added a commit that referenced this issue Feb 13, 2024
Just run this on each PR to flag any changes. Since `main` is working toward a
2.0 release, we should be ok making breaking changes, but it likely doesn't hurt
to realize what the breaks are.

Used some of the https://github.com/vapor projects for reference and the git
magic needed to for the action to run.

Fixes #1026
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 a pull request may close this issue.

3 participants