[WIP] protobuf: build with cmake instead of autoconf #84824

eamsden · 2020-04-09T15:40:55Z

Motivation for this change

Protobuf has both cmake and autoconf/Make based build systems. The CMake build system will install both pkgconfig and cmake packages for other builds using protobuf to import, while the autoconf build system only installs the pkgconfig files.

In particular, this is a prerequisite for packaging the onnx library

Things done

FRidh · 2020-04-10T10:14:44Z

cc @veprbl who has a protobuf PR open as well

nh2 · 2020-04-10T13:19:01Z

cc @veprbl who has a protobuf PR open as well

That other PR is #84867.

veprbl · 2020-04-10T16:57:03Z

Thanks for the ping. I don't mind either solution.

veprbl · 2020-04-15T17:55:26Z

@eamsden please rebase on top of master.

…is exported

eamsden · 2020-04-15T18:04:37Z

@veprbl done

veprbl · 2020-04-15T18:06:27Z

pkgs/development/libraries/protobuf/protobuf-cmake.patch

+
+-install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_INSTALL_CMAKEDIR}/
+install(DIRECTORY ${CMAKE_INSTALL_CMAKEDIR}/
+   DESTINATION "${CMAKE_INSTALL_CMAKEDIR}"


I suspect you are trying to deal with an absolute path we set in our default setup-hook:

nixpkgs/pkgs/development/tools/build-managers/cmake/setup-hook.sh

Line 81 in 2ef110a

cmakeFlags="-DCMAKE_INSTALL_LIBDIR=${!outputLib}/lib $cmakeFlags"

But how can this work that you install from a directory in "$out" to itself.
I think, we typically workaround this by doing:

nixpkgs/pkgs/development/libraries/libfido2/default.nix

Line 45 in 2ef110a

"-DCMAKE_INSTALL_LIBDIR=lib"

Is it preferred to do it through flags instead of patching the (very odd and unfortunate) CMake call?

The good thing about "-DCMAKE_INSTALL_LIBDIR=lib" is that we can search for it through nixpkgs and see packages affected.

Also, I am still not sure how your patch works.

Looking at the source code, it intends to put intermediary file generated by configure_file under lib/cmake/protobuf in the build directory but (wrongly) assumes the CMAKE_INSTALL_LIBDIR is relative.

The line you patch would install the directory from the build directory to the target location but since we have absolute CMAKE_INSTALL_LIBDIR, it is already in the target location and not in the build directory (${CMAKE_CURRENT_BINARY_DIR}).

The correct patch would be changing the output of configure_file and the input of install(DIRECTORY to something like ${CMAKE_CURRENT_BINARY_DIR}/this_is_an_intermediary_dir_for_cmake_files. I do not think there is any need for the intermediate directory to be the same as the target destination.

veprbl · 2020-04-15T18:12:34Z

pkgs/development/libraries/protobuf/generic-v3.nix

+    # CMake doesn't generate the version.rc file from version.rc.in.
+    ++ lib.optional
+       (lib.versionAtLeast version "3.6" && lib.versionOlder version "3.7")
+       ./protobuf-3.6-cmake-versionrc.patch;


Upstream only generates that one for windows:
https://github.com/protocolbuffers/protobuf/blob/7b8a241b575b756c4dd7a5aec7f7a60854d5f711/cmake/CMakeLists.txt#L219-L220
Why do we need it? If we need it, we should let upstream know by submitting the patch or filing an issue.

It causes the build to fail, but just for version 3.6, if we don't generate it, thus the version bounding check.

pkgs/development/libraries/protobuf/protobuf-cmake.patch

veprbl · 2020-04-15T18:20:02Z

pkgs/development/libraries/protobuf/generic-v3.nix

+    (if lib.versionAtLeast version "3.6.0"
+     then ''
+       rm -rf third_party/googletest
+       cp -a ${gmock.src} third_party/googletest
+       chmod -R a+w third_party/googletest
+       ''
+     else ''
+     cp -a ${gmock.src}/googlemock gmock
+     cp -a ${gmock.src}/googletest googletest
+     chmod -R a+w gmock
+     chmod -R a+w googletest
+     ln -s ../googletest gmock/gtest
+     '')


Can set fetchSubmodules = true in fetchFromGithub (don't forget to update hashes).

That is an approach I could take. I borrowed this approach from the existing Autoconf build.

Let's try that. Also remove the darwin hack below, we'll figure something out if it's still needed.

(Don't forget to drop gmock argument)

veprbl · 2020-04-15T18:22:40Z

pkgs/development/libraries/protobuf/generic-v3.nix

  buildInputs = [ zlib ];
-  configureFlags = if buildProtobuf == null then [] else [ "--with-protoc=${buildProtobuf}/bin/protoc" ];


Did you check that cross builds still function?

I believe so. protobuf builds protoc but doesn't actually use it as part of the build, it appears someone misread the instructions for cross compiling projects that depend on protobuf as directions for cross-compiling protobuf. But I'll run the build one more time to be sure.

nix-build -A pkgsCross.armv7l-hf-multiplatform.protobuf is now failing with a not found rather than a exec format error for protoc (the latter would have been what I expected. I'll investigate in a few hours.

veprbl · 2020-04-15T18:37:13Z

pkgs/development/libraries/protobuf/generic-v3.nix


  enableParallelBuilding = true;
-
  doCheck = true;

  dontDisableStatic = true;


This option is only applicable to the default configurePhase:

nixpkgs/pkgs/stdenv/generic/setup.sh

Lines 981 to 986 in 2ef110a

# By default, disable static builds.

if [ -z "${dontDisableStatic:-}" ]; then

if [ -f "$configureScript" ] && grep -q enable-static "$configureScript"; then

configureFlags="--disable-static $configureFlags"

fi

fi

I would check if static libraries are still present and remove this line. In fact we should compare what files are present in the old build result vs the new.

veprbl · 2020-04-15T18:37:44Z

pkgs/development/libraries/protobuf/generic-v3.nix

  buildInputs = [ zlib ];
-  configureFlags = if buildProtobuf == null then [] else [ "--with-protoc=${buildProtobuf}/bin/protoc" ];

  enableParallelBuilding = true;


This line is not needed for cmake builds, as it is the default there.

pkgs/development/libraries/protobuf/generic-v3.nix

veprbl · 2020-04-19T15:54:10Z

~~Also, please rebase to staging.~~ already done

FRidh · 2020-05-08T04:44:43Z

Could you please resolve the conversations and explicitly approve. That way I know when a PR is ready for staging. Thanks!

jtojnar · 2020-05-08T05:31:53Z

pkgs/development/libraries/protobuf/generic-v3.nix

-      --replace 'tmpnam(b)' '"'$TMPDIR'/foo"'
-  '';
+  patches =
+    [ ./protobuf-cmake.patch ]


Could you add a comment and link to upstream issue/PR?

jtojnar · 2020-05-08T05:37:33Z

pkgs/development/libraries/protobuf/generic-v3.nix

-let
-mkProtobufDerivation = buildProtobuf: stdenv: stdenv.mkDerivation {
+stdenv.mkDerivation rec {
+  # make sure you test also -A pythonPackages.protobuf


Maybe add

passthru = { tests = { python = python3.pkgs.protobuf; }; };

Ofborg will then check the python module automatically.

stale · 2021-06-04T14:02:52Z

I marked this as stale due to inactivity. → More info

tobim · 2024-02-06T06:03:22Z

This has been implemented in #169497.

ofborg bot added 11.by: package-maintainer 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 501-1000 labels Apr 9, 2020

veprbl added the 2.status: merge conflict label Apr 10, 2020

veprbl linked an issue Apr 10, 2020 that may be closed by this pull request

closure size: protobuf depends on C++ compiler #73919

Open

ofborg bot removed the 2.status: merge conflict label Apr 15, 2020

protobuf: build with cmake instead of autoconf so that cmake package …

0351abd

…is exported

eamsden force-pushed the eamsden/protobuf-build-with-cmake branch from 7bb0771 to 0351abd Compare April 15, 2020 18:04

veprbl reviewed Apr 15, 2020

View reviewed changes

pkgs/development/libraries/protobuf/protobuf-cmake.patch Show resolved Hide resolved

veprbl reviewed Apr 15, 2020

View reviewed changes

pkgs/development/libraries/protobuf/generic-v3.nix Show resolved Hide resolved

nh2 mentioned this pull request Apr 21, 2020

protobuf: do a release build (without -g) #84867

Closed

10 tasks

FRidh changed the base branch from master to staging April 27, 2020 18:13

veprbl changed the title ~~protobuf: build with cmake instead of autoconf~~ [WIP] protobuf: build with cmake instead of autoconf May 8, 2020

jtojnar reviewed May 8, 2020

View reviewed changes

veprbl added the 9.needs: reporter feedback label May 23, 2020

ryantm marked this pull request as draft October 23, 2020 03:01

stale bot added the 2.status: stale label Jun 4, 2021

veprbl added the 2.status: merge conflict label Jan 9, 2022

stale bot removed the 2.status: stale label Jan 9, 2022

stale bot added the 2.status: stale label Jul 10, 2022

tobim closed this Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] protobuf: build with cmake instead of autoconf #84824

[WIP] protobuf: build with cmake instead of autoconf #84824

eamsden commented Apr 9, 2020

FRidh commented Apr 10, 2020

nh2 commented Apr 10, 2020

veprbl commented Apr 10, 2020

veprbl commented Apr 15, 2020

eamsden commented Apr 15, 2020

veprbl Apr 15, 2020

eamsden Apr 15, 2020

veprbl Apr 15, 2020

jtojnar Apr 19, 2020

veprbl Apr 15, 2020

eamsden Apr 15, 2020

veprbl Apr 15, 2020

eamsden Apr 15, 2020

veprbl Apr 15, 2020

veprbl Apr 15, 2020

veprbl Apr 15, 2020

eamsden Apr 15, 2020

eamsden Apr 15, 2020

veprbl Apr 15, 2020

veprbl Apr 15, 2020

veprbl commented Apr 19, 2020 •

edited

Loading

FRidh commented May 8, 2020

jtojnar May 8, 2020

jtojnar May 8, 2020

stale bot commented Jun 4, 2021

tobim commented Feb 6, 2024

		buildInputs = [ zlib ];
		configureFlags = if buildProtobuf == null then [] else [ "--with-protoc=${buildProtobuf}/bin/protoc" ];

	# By default, disable static builds.
	if [ -z "${dontDisableStatic:-}" ]; then
	if [ -f "$configureScript" ] && grep -q enable-static "$configureScript"; then
	configureFlags="--disable-static $configureFlags"
	fi
	fi

[WIP] protobuf: build with cmake instead of autoconf #84824

[WIP] protobuf: build with cmake instead of autoconf #84824

Conversation

eamsden commented Apr 9, 2020

Motivation for this change

Things done

FRidh commented Apr 10, 2020

nh2 commented Apr 10, 2020

veprbl commented Apr 10, 2020

veprbl commented Apr 15, 2020

eamsden commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veprbl commented Apr 19, 2020 • edited Loading

FRidh commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 4, 2021

tobim commented Feb 6, 2024

veprbl commented Apr 19, 2020 •

edited

Loading