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.

Sign up for GitHub

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

Jump to bottom

protoc-gen-swift: init at 1.28.2 #321607

Merged
merged 1 commit into from
Nov 12, 2024
Merged

protoc-gen-swift: init at 1.28.2 #321607

merged 1 commit into from
Nov 12, 2024

Conversation

matteo-pacini
Copy link
Contributor

@matteo-pacini matteo-pacini commented Jun 21, 2024 •
edited
Loading

Description of changes

Protobuf plugin for generating Swift code.

https://github.com/apple/swift-protobuf

Extra notes:

  • This cannot be backported because the Swift version on release-24.05 is too old to support this package.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@matteo-pacini matteo-pacini mentioned this pull request Jun 21, 2024
Closed
@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Jun 21, 2024
@matteo-pacini
Copy link
Contributor Author

Result of nixpkgs-review pr 321607 run on aarch64-darwin 1

1 package built:
  • protoc-gen-swift

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4132

afh
afh reviewed Jun 23, 2024
View reviewed changes
Copy link
Member

@afh afh 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 this, @matteo-pacini, please find below a few questions, ideas, and suggestions for improvement. Hopefully I've been able to describe understandably what I mean, if not please ask 🙂

@afh
Copy link
Member

afh commented Jun 23, 2024

Result of nixpkgs-review pr 321607 run on aarch64-darwin 1

1 package built:
  • protoc-gen-swift

@matteo-pacini
Copy link
Contributor Author

@afh thanks for the feedback, ready for re-review 👍🏻

afh
afh reviewed Jun 23, 2024
View reviewed changes
Copy link
Member

@afh afh 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 addressing the review feedback, @matteo-pacini, much appreciated. During re-review I wondered about the following comment. What are your thoughts?

@matteo-pacini
Copy link
Contributor Author

@afh ready for re-review 🙏🏻

afh
afh approved these changes Jun 23, 2024
View reviewed changes
Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

Nicely done, @matteo-pacini 🏅

@ofborg ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 23, 2024
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 23, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1779

@SuperSandro2000
Copy link
Member

This seems to fail to build for every platform.

@afh
Copy link
Member

afh commented Jun 27, 2024

The darwin builds fail due to error: building of '/nix/store/9qd437z1qq2h1fvch8p88jdn80vmn72r-swift-5.8.drv^lib,man,out' from .drv file timed out after 3600 seconds

The aarch64-linux build fails with <module-includes>:1:10: note: in file included from <module-includes>:1: #include "CoreFoundation.h"

The x86_64-linux build fails due to:

error: 'source': Invalid manifest (compiled with: ["/nix/store/jlpi1wiiqknd9jdqlwvi330j89wjy6b4-swift-wrapper-5.8/bin/swiftc", "-vfsoverlay", "/build/TemporaryDirectory.yxBOXV/vfs.yaml", "-L", "/nix/store/z4i1a12x0d5gqv2zxlcrvb8676mlxb87-swiftpm-5.8/lib/swift/pm/ManifestAPI", "-lPackageDescription", "-Xlinker", "-rpath", "-Xlinker", "/nix/store/z4i1a12x0d5gqv2zxlcrvb8676mlxb87-swiftpm-5.8/lib/swift/pm/ManifestAPI", "-swift-version", "5", "-I", "/nix/store/z4i1a12x0d5gqv2zxlcrvb8676mlxb87-swiftpm-5.8/lib/swift/pm/ManifestAPI", "-package-description-version", "5.6.0", "/build/source/[email protected]", "-o", "/build/TemporaryDirectory.ALmySn/source-manifest"])
<unknown>:0: error: could not find module '_Concurrency' for target 'x86_64-pc-linux-gnu'; found: x86_64-unknown-linux-gnu, at: /nix/store/gl7r836wv97a12pvawrgz60f2v3gvk9r-swift-5.8-lib/lib/swift/linux/_Concurrency.swiftmodule

At first glance I'd like to assume that this is due to Swift build failures rather than failures introduced with the changes proposed in this PR.

@matteo-pacini
Copy link
Contributor Author

Yup, this one is probably parked until #320900 is resolved

@matteo-pacini
Copy link
Contributor Author

@ofborg eval

@matteo-pacini matteo-pacini marked this pull request as draft July 18, 2024 18:54
@matteo-pacini matteo-pacini changed the title protoc-gen-swift: init at 1.26.0 protoc-gen-swift: init at 1.28.2 Nov 9, 2024
@matteo-pacini matteo-pacini marked this pull request as ready for review November 9, 2024 21:14
@matteo-pacini
Copy link
Contributor Author

@afh Resurrecting this PR as Swift seems to work okay now.

@matteo-pacini matteo-pacini requested a review from afh November 9, 2024 21:16
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4818

@afh
Copy link
Member

afh commented Nov 9, 2024

Result of nixpkgs-review pr 321607 run on aarch64-darwin 1

1 package built:
  • protoc-gen-swift

afh
afh approved these changes Nov 9, 2024
View reviewed changes
Copy link
Member

@afh afh 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 your persistence on this, @matteo-pacini, looking good from my end 👍

Let's see what CI reveals…

@matteo-pacini matteo-pacini force-pushed the protoc-gen-swift branch 2 times, most recently from 288572b to ef9eb80 Compare November 10, 2024 09:07
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 10, 2024
@matteo-pacini
Copy link
Contributor Author

@ofborg eval

@SuperSandro2000
Copy link
Member

@ofborg build protoc-gen-swift

@matteo-pacini
Copy link
Contributor Author

@SuperSandro2000 should work now, thanks!

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 321607

x86_64-linux

✅ 1 package built:
  • protoc-gen-swift

x86_64-darwin

✅ 1 package built:
  • protoc-gen-swift

aarch64-darwin

✅ 1 package built:
  • protoc-gen-swift

@matteo-pacini matteo-pacini added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 11, 2024
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 11, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2111

@SuperSandro2000 SuperSandro2000 merged commit d78511d into NixOS:master Nov 12, 2024
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afh afh afh approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matteo-pacini @nixos-discourse @afh @SuperSandro2000 @wegank