Skip to content

Conversation

@mkruskal-google
Copy link
Member

This removes some cyclic logic in our installation tests, where we select which headers to remove from our CMake file_lists. We then build and run our tests, expecting our previously installed headers to get picked up. Since these lists are also used to determine which headers get installed, the fact that port_def.inc was excluded from installation was missed.

Instead, with this PR we glob and remove all *.h and *.inc* files that haven't been explicitly grouped as test utilities.

Copy link
Contributor

@devjgm devjgm left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

Not blocking this PR, but a question: Do we have an "integration test" that executes a few bash commands exactly like a user should execute them to verify that the installed artifacts work? I'd imagine a several line bash script like:

# Build and install Protobuf in /tmp/installed
cmake -S. -B/tmp/build -DCMAKE_INSTALL_PREFIX=/tmp/installed -D...
cmake --build /tmp/build
cmake --build /tmp/build --target install

# Build a simple test program that links against the installed protobuf
...

Do we have something like this?

@mkruskal-google
Copy link
Member Author

My original plan was to create a test like that, but the current setup seemed to have a lower maintenance cost and better coverage. Basically we do three steps:

  1. Install protobuf (and assert there isn't already an instance installed)
  2. Delete all the local artifacts that should have been installed (e.g. build artifacts, headers, etc)
  3. Build and run our test suite without rebuilding protoc or libprotobuf

Basically, it reuses our test suite as that "simple test program"

@mkruskal-google mkruskal-google merged commit 81e3513 into protocolbuffers:main Sep 19, 2022
@mkruskal-google mkruskal-google deleted the cmake_install2 branch September 27, 2022 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants