Navigation Menu

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

Jump to bottom

nanopb: init at 0.4.1 #89070

Merged
merged 2 commits into from Jun 16, 2020
Merged

nanopb: init at 0.4.1 #89070

merged 2 commits into from Jun 16, 2020

Conversation

kalbasit
Copy link
Member

Motivation for this change

Nanopb 0.4.1 is a small code-size Protocol Buffers implementation in ansi C. It is especially suitable for use in microcontrollers, but fits any memory restricted system.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@kalbasit kalbasit marked this pull request as draft May 28, 2020 02:08
@kalbasit kalbasit marked this pull request as ready for review May 28, 2020 02:22
@kalbasit kalbasit requested review from Mic92 and jonringer May 28, 2020 16:45
@jonringer
Copy link
Contributor

interesting way to create tests... not sure if i love or hate it :)

@kalbasit
Copy link
Member Author

interesting way to create tests... not sure if i love or hate it :)

Would you like to discuss your concerns? It'd be great to get this merged soon.

Mic92
Mic92 reviewed May 29, 2020
View reviewed changes
pkgs/development/libraries/nanopb/default.nix
simple-proto2 = callPackage ./test-simple-proto2 {};
simple-proto3 = callPackage ./test-simple-proto3 {};
message-with-annotations = callPackage ./test-message-with-annotations {};
message-with-options = callPackage ./test-message-with-options {};
Copy link
Member

Choose a reason for hiding this comment

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

I think @Profpatsch had a similar idea once.

@Mic92
Copy link
Member

Mic92 commented May 29, 2020 •
edited

interesting way to create tests... not sure if i love or hate it :)

Would you like to discuss your concerns? It'd be great to get this merged soon.

I would split-off the test part of this PR. Using passthru.tests is an interesting idea. However I would like to discuss if we should separate nixosTests from other test derivations. If you break change into two commits, I would cherry-pick one of them two masters.

@kalbasit
Copy link
Member Author

kalbasit commented May 29, 2020 •
edited

interesting way to create tests... not sure if i love or hate it :)

Would you like to discuss your concerns? It'd be great to get this merged soon.

I would split-off the test part of this PR. Using passthru.tests is an interesting idea. However I would like to discuss if we should separate nixosTests from other test derivations. If you break change into two commits, I would cherry-pick one of them two masters.

This isn't the first package to do so:

Among others in git grep passthru.tests -- pkgs/**/default.nix

Also, ofborg has native support for {pkg}.tests now and it always tries to build {pkg} and {pkg}.tests. For instance, the checks for this PR.

@kalbasit
Copy link
Member Author

kalbasit commented Jun 5, 2020

@Mic92 I split the commits as suggested. PTAL.

@kalbasit kalbasit requested a review from Mic92 June 6, 2020 04:14
@kalbasit kalbasit merged commit 15a059b into NixOS:master Jun 16, 2020
@jonringer
Copy link
Contributor

@kalbasit sorry for not responding earlier.

The love part is having additional test which validate a package, the hate part is the amount of nix scaffolding needed to make it happen.

@kalbasit
Copy link
Member Author

@kalbasit sorry for not responding earlier.

No worries! Things can get busy at times, and it's completely understandable!

The love part is having additional test which validate a package, the hate part is the amount of nix scaffolding needed to make it happen.

I get your point. I've had a bad experience with protoc and its plugins in the past. I want to make sure that updating the package does not just rely on the build passing but actually asserts that it works correctly. Nanopb itself does provide tests, I'll explore that and see if I can swap out our tests in favor of those.

@kalbasit kalbasit deleted the nixpkgs_add-nanopb branch June 17, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer Awaiting requested review from jonringer

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kalbasit @jonringer @Mic92