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

v21.0 does not install all the CMake configuration files #10045

Closed
coryan opened this issue May 25, 2022 · 9 comments
Closed

v21.0 does not install all the CMake configuration files #10045

coryan opened this issue May 25, 2022 · 9 comments
Labels

Comments

@coryan
Copy link
Contributor

coryan commented May 25, 2022

What version of protobuf and what language are you using?

Version: v21.0
Language: C++

What operating system (Linux, Windows, ...) and version?

Linux, but probably all operating systems are affected

What runtime / compiler are you using (e.g., python version or gcc version)

 gcc --version
gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

What did you do?

Compiled and installed protobuf using CMake. See this Dockerfile.txt

What did you expect to see

A working install.

What did you see instead?

Protobuf did not install the protobuf-config.cmake file, and thus find_package(protobuf CONFIG) is not functional.

Anything else we should know about your project / environment

@acozzette
Copy link
Member

@jonringer I suspect this bug may be related to #9822. Do you have any thoughts on this?

acozzette added a commit to acozzette/protobuf that referenced this issue May 26, 2022
This reverts commit c80808c.

Thix fixes protocolbuffers#10045. Somehow the change caused these four .cmake files to
stop being installed:

protobuf-config.cmake
protobuf-config-version.cmake
protobuf-module.cmake
protobuf-options.cmake

After reverting the change, I confirmed that the files are being
installed again.
@acozzette
Copy link
Member

@jonringer I am going to revert #9822 for now to fix the regression in 3.21.0. I would be happy to merge some version of the original fix again if you can figure out how to avoid the problem in this issue, though.

acozzette added a commit that referenced this issue May 26, 2022
This reverts commit c80808c.

Thix fixes #10045. Somehow the change caused these four .cmake files to
stop being installed:

protobuf-config.cmake
protobuf-config-version.cmake
protobuf-module.cmake
protobuf-options.cmake

After reverting the change, I confirmed that the files are being
installed again.
@jonringer
Copy link
Contributor

21.0 does install the related files

/nix/store/6h46s3n81xmsq2ag3rcddsaspw6dq66s-protobuf-3.21.0/lib/cmake
└── protobuf
    ├── protobuf-config.cmake
    ├── protobuf-config-version.cmake
    ├── protobuf-module.cmake
    ├── protobuf-options.cmake
    ├── protobuf-targets.cmake
    └── protobuf-targets-release.cmake

@jonringer
Copy link
Contributor

potential problem is that it's installed under a protobuf directory, however, I just tried to emulate the existing install behavior. The only reason why it worked pre #9822 was because the build was installing the targets. So by reverting #9822, it's now more broken

@jonringer
Copy link
Contributor

I'm on my honeymoon, I can come around to this in a few days; but I'm sure the fix will just be to remove the contents from the protobuf up to the cmake directory

@coryan
Copy link
Contributor Author

coryan commented May 27, 2022

AFAIK, on nixOS things like CMAKE_INSTALL_CMAKEDIR are absolute paths, so these lines:

configure_file(${protobuf_SOURCE_DIR}/cmake/protobuf-config.cmake.in
${CMAKE_INSTALL_CMAKEDIR}/protobuf-config.cmake @ONLY)
configure_file(${protobuf_SOURCE_DIR}/cmake/protobuf-config-version.cmake.in
${CMAKE_INSTALL_CMAKEDIR}/protobuf-config-version.cmake @ONLY)
configure_file(${protobuf_SOURCE_DIR}/cmake/protobuf-module.cmake.in
${CMAKE_INSTALL_CMAKEDIR}/protobuf-module.cmake @ONLY)
configure_file(${protobuf_SOURCE_DIR}/cmake/protobuf-options.cmake
${CMAKE_INSTALL_CMAKEDIR}/protobuf-options.cmake @ONLY)

Directly create the file in the destination directory (/nix/store/6h46s3n81xmsq2ag3rcddsaspw6dq66s-protobuf-3.21.0/lib/cmake in your example). In other distributions and platforms CMAKE_INSTALL_CMAKEDIR is a relative directory. In those other platforms, the previous lines create the files in ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_INSTALL_CMAKEDIR}, and they are copied from that (temporary) location to the install destination by these lines:

install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_INSTALL_CMAKEDIR}/
DESTINATION "${CMAKE_INSTALL_CMAKEDIR}"
COMPONENT protobuf-export
PATTERN protobuf-targets.cmake EXCLUDE
)

Those latter lines are removed by #9822. In other words,#9822 fixed some things for nixOS, and broke other things in other distributions/platforms.

PS: all of this can wait until you return, enjoy your honeymoon.

@jonringer
Copy link
Contributor

and they are copied from that (temporary) location to the install destination by these lines:

Yes, but the copied files will have targets paths which refer to the build directory, and this isn't desirable either.

@coryan
Copy link
Contributor Author

coryan commented May 28, 2022

and they are copied from that (temporary) location to the install destination by these lines:

Yes, but the copied files will have targets paths which refer to the build directory, and this isn't desirable either.

First, I am not arguing that the current code is correct, I agree it needs fixing to support nixOS. I am arguing that the code in #9822 was definitely wrong for other platforms. I tried to explain why it was incorrect, so the next iteration can avoid that pitfall.

Second, I may be reading your comment about references to the build directory wrong. It does not match the observed behavior in my testing. Naturally this may depend on how protobuf is configured and installed, and I just happen to use the right combination of options that "works" 🤷

# syntax=docker/dockerfile:1.3-labs
# test using
#   docker buildx build --progress plain -t test - <Dockerfile

FROM ubuntu:22.04

# Install development tools
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update && \
    apt-get --no-install-recommends install -y apt-transport-https apt-utils \
        automake build-essential ccache cmake ca-certificates curl git \
        gcc g++ libc-ares-dev libc-ares2 libcurl4-openssl-dev libre2-dev \
        libssl-dev m4 make ninja-build pkg-config tar wget zlib1g-dev


# Compile and install Abseil
WORKDIR /var/tmp/build/abseil-cpp
RUN curl -sSL https://github.com/abseil/abseil-cpp/archive/20211102.0.tar.gz | \
    tar -xzf - --strip-components=1 && \
    cmake \
      -DCMAKE_BUILD_TYPE=Release \
      -DBUILD_TESTING=OFF \
      -DBUILD_SHARED_LIBS=yes \
      -DCMAKE_CXX_STANDARD=11 \
      -G Ninja -S . -B.build && \
    cmake --build .build && \
    cmake --build .build --target install && \
    ldconfig

# Compile and install Protobuf
WORKDIR /var/tmp/build/protobuf-testonly-z4OsGXYoPj
RUN curl -sSL https://github.com/protocolbuffers/protobuf/archive/v21.1.tar.gz | \
    tar -xzf - --strip-components=1 && \
    cmake \
        -DCMAKE_BUILD_TYPE=Release \
        -DBUILD_SHARED_LIBS=yes \
        -Dprotobuf_BUILD_TESTS=OFF \
        -Dprotobuf_ABSL_PROVIDER=package \
        -G Ninja -S . -B.build && \
    cmake --build .build && \
    cmake --build .build --target install  && \
    ldconfig

RUN if find /usr/local/lib/cmake/protobuf -type f xargs grep testonly-z4OsGXYoPj; then false; fi

Again, I understand the code in v21.1 does not work for nixOS. That certainly needs fixing. I think you would agree that the fixes should keep things working on other Linux distributions, and #9822 does not achieve that.

@dailylama
Copy link

21.0 does install the related files

/nix/store/6h46s3n81xmsq2ag3rcddsaspw6dq66s-protobuf-3.21.0/lib/cmake
└── protobuf
    ├── protobuf-config.cmake
    ├── protobuf-config-version.cmake
    ├── protobuf-module.cmake
    ├── protobuf-options.cmake
    ├── protobuf-targets.cmake
    └── protobuf-targets-release.cmake

if I add_subdirectory protobuf from other projects cmake file, these files end up in following directories, is there any way to reference files built to run find_package ?

build/cmake/protobuf/protobuf-config.cmake
build/CMakeFiles/Export/5a0f10ea4324979995731323bb365f6f/protobuf-targets.cmake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants