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

protobuf3_21: 3.21.2 -> 21.5, update versioning scheme #191207

Closed
wants to merge 1 commit into from

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Sep 14, 2022

Description of changes

Upstream dropped the previous "3" major version, to better help with handling changes to underlying ecosystems. https://developers.google.com/protocol-buffers/docs/news/2022-05-06

cc @infinisil if this is the correct way to do this version transition

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Sep 14, 2022
@@ -46,7 +46,7 @@ let
--replace 'tmpnam(b)' '"'$TMPDIR'/foo"'
'';

patches = lib.optionals (lib.versionOlder version "3.22") [
patches = lib.optionals (lib.versionOlder version "22") [
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still work like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

protobuf 3.21 was the only one using the cmake generic builder. So yes :).

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

IMO we should also try to reduce the number of protobuf variants we have.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@jonringer
Copy link
Contributor Author

IMO we should also try to reduce the number of protobuf variants we have.

I think the maintenance is pretty minimal. And protobuf is pretty pedantic about which versions you use, so I think we should keep around the non-EOL versions.

@superherointj
Copy link
Contributor

New version, v21.6, is available:
https://github.com/protocolbuffers/protobuf/releases/tag/v21.6

@Mindavi
Copy link
Contributor

Mindavi commented Sep 26, 2022

When I try to cross-compile, I get this:

rick@nixos-asus:~/nixpkgs$ nix build .#pkgsCross.aarch64-multiplatform.protobuf_21
error: attribute 'protobuf21_5' missing

       at /nix/store/pycf5k246bmfz21fnl96a2m4yxdl84wy-source/pkgs/development/libraries/protobuf/generic-v3-cmake.nix:65:7:

           64|       # re-use the executable generated as part of the build
           65|       buildPackages."protobuf${protobufVersion}"
             |       ^
           66|     ];
(use '--show-trace' to show detailed location information)

If I add the alias protobuf21_5, it works.

@tjni
Copy link
Contributor

tjni commented Sep 27, 2022

This could fix several errors12 in #191339 because it picks up protocolbuffers/protobuf#10271.

From my reading of the ticket, it seems that protobufc is built in debug mode but protobuf isn't? I don't know enough about building C++ programs to know for sure.

Footnotes

  1. https://malob.github.io/nix-review-tools-reports/nixpkgs:staging-next/nixpkgs_staging-next_1782066.html

  2. https://hydra.nixos.org/build/191476697

@jonringer
Copy link
Contributor Author

If I add the alias protobuf21_5, it works.

Yea.... cross compilation + overriding :(

@Mindavi
Copy link
Contributor

Mindavi commented Sep 27, 2022

FYI, it's these lines that cause this to happen:

protobufVersion = "${lib.versions.major version}_${lib.versions.minor version}";
in [
cmake
] ++ lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [
# protoc of the same version must be available for build. For non-cross builds, it's able to
# re-use the executable generated as part of the build
buildPackages."protobuf${protobufVersion}"

Maybe still using the minor version in the name would be good? Otherwise those lines should probably be changed to support both.

@jonringer
Copy link
Contributor Author

Seeing as 3.21/21.x are the only versions of protobuf using the cmake builder, I can just adopt the newer convention.

@vcunat
Copy link
Member

vcunat commented Sep 29, 2022

Note: 3.21 got security update in the meantime (and became the default version).

@superherointj
Copy link
Contributor

superherointj commented Sep 30, 2022

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 1, 2022
@sagikazarmark
Copy link
Member

Can I help with anything to move this forward?

Also, would it make sense to rename/add a new package called protoc? Given that the new versioning scheme is for protoc only and language runtimes will continue using semver.

@jonringer
Copy link
Contributor Author

After 6 months, I stopped caring, let's get this in.

@jonringer
Copy link
Contributor Author

Updated diff, going to merge in 24hrs if no objections. It's been ~6 months

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 21, 2023

callPackage ./generic-v3-cmake.nix {
version = "21.12";
sha256 = "sha256-VZQEFHq17UsTH5CZZOcJBKiScGV2xPJ/e6gkkVliRCU=";
Copy link
Member

Choose a reason for hiding this comment

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

Migrate this to hash, please.

@@ -0,0 +1,6 @@
{ callPackage, abseil-cpp, ... }:

callPackage ./generic-v3-cmake.nix {
Copy link
Member

Choose a reason for hiding this comment

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

Is v3 here still correct/relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can probably remove it. shouldn't be meaningful anymore.

@jonringer
Copy link
Contributor Author

much of this was applied

@jonringer jonringer closed this Nov 23, 2023
@jonringer jonringer deleted the bump-protobuf-21 branch November 23, 2023 17:45
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-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants