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

modern-cpp-kafka: init at 2023.03.07 #251825

Merged
merged 2 commits into from
Oct 16, 2023
Merged

Conversation

ditsuke
Copy link
Member

@ditsuke ditsuke commented Aug 27, 2023

Description of changes

Adds a derivation for modern-cpp-kafka.

NOTE
Using my fork that patches some cmake build setup temporarily. Won't be required once my PR
is accepted upstream. Alternatively I could apply my fixes as an inline-patch while still using the official repo.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

pkgs/development/libraries/modern-cpp-kafka/default.nix Outdated Show resolved Hide resolved
# Patch build to avoid overwriting include path
# PR: https://github.com/morganstanley/modern-cpp-kafka/pull/221
owner = "ditsuke";
rev = "build/avoid-overwriting-include-path";
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the upstream version of this project anyway and add your patches via the patches ? Or, since your PR is still very young, maybe just wait a bit and see if they accept it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The build won't work without it, but I've added it as a patch for now.

pkgs/development/libraries/modern-cpp-kafka/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/modern-cpp-kafka/default.nix Outdated Show resolved Hide resolved
nativeBuildInputs = [ cmake ];

buildPhase = ''
cmake .
Copy link
Member

Choose a reason for hiding this comment

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

Calls to cmake, which is a configuration tool, do not belong into the buildPhase.
Do you intent to just provide the headers with this library, or do you also plan to ship compiled library files also? For only header files, you may not even need a configuration phase. But that should be handled by having cmake in nativeBuildInputs already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just the headers

pkgs/development/libraries/modern-cpp-kafka/default.nix Outdated Show resolved Hide resolved
@ditsuke ditsuke force-pushed the add/modern-cpp-kafka branch 2 times, most recently from 7ba7bb7 to 9c6bc60 Compare August 28, 2023 10:47
pkgs/development/libraries/modern-cpp-kafka/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/modern-cpp-kafka/default.nix Outdated Show resolved Hide resolved
maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
@nagy
Copy link
Member

nagy commented Aug 28, 2023

Oh and one more thing, can you change the commit message ( and the PR title) to modern-cpp-kafka: init at 2023.03.07 ?

@ditsuke ditsuke force-pushed the add/modern-cpp-kafka branch from 9c6bc60 to e78f9fc Compare August 28, 2023 18:41
@ditsuke ditsuke changed the title modern-cpp-kakfa: init modern-cpp-kakfa: init at 2023.03.07 Aug 28, 2023
@nagy
Copy link
Member

nagy commented Aug 28, 2023

A few things are still left.

  • The commit message ( and PR title) is still named "kakfa" instead of "kafka".

From here it gets pedantic, but we want to keep the commit history in a good quality:

After that it should be good to go.

@ditsuke ditsuke changed the title modern-cpp-kakfa: init at 2023.03.07 modern-cpp-kafka: init at 2023.03.07 Aug 29, 2023
@ditsuke ditsuke force-pushed the add/modern-cpp-kafka branch 3 times, most recently from b4e3293 to f8c3904 Compare August 30, 2023 07:40
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Aug 30, 2023
@ditsuke
Copy link
Member Author

ditsuke commented Aug 30, 2023

A few things are still left.

* The commit message ( and PR title) is still named "kakfa" instead of "kafka".

From here it gets pedantic, but we want to keep the commit history in a good quality:

* The commit message of the maintainership commit does not fit. You can find other good examples in the history of that file: [`master`/maintainers/maintainer-list.nix (commits)](https://github.com/NixOS/nixpkgs/commits/master/maintainers/maintainer-list.nix)

* Therefore, the maintainership commit should be separate from any packages and you can just directly write it into the package commit itself.

After that it should be good to go.

@nagy split and reworded the maintainer commit, I hope it's good to go now

@nagy
Copy link
Member

nagy commented Aug 30, 2023

The commit message is still "kakfa" . After that, its good.

@ditsuke ditsuke force-pushed the add/modern-cpp-kafka branch from f8c3904 to 29d2997 Compare August 30, 2023 09:38
@ditsuke
Copy link
Member Author

ditsuke commented Aug 30, 2023

The commit message is still "kakfa" . After that, its good.

Whoops, sorry about that. Good to go

@nagy nagy requested a review from marsam August 30, 2023 11:08
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 6, 2023
@nagy nagy requested a review from wegank October 15, 2023 20:06
@wegank wegank marked this pull request as draft October 15, 2023 21:30
@wegank wegank force-pushed the add/modern-cpp-kafka branch from 29d2997 to 568c127 Compare October 15, 2023 21:30
@wegank wegank marked this pull request as ready for review October 15, 2023 21:31
@wegank
Copy link
Member

wegank commented Oct 16, 2023

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

1 package built:
  • modern-cpp-kafka

@wegank wegank merged commit 7ac9890 into NixOS:master Oct 16, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants