-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
glog: 0.4.0 -> 0.5.0, also enable tests #144561
Conversation
@tobim Would you be up to be co-maintainer on the |
Example
The sought file was built but apparently not installed:
Indeed it's missing in here:
That looks wrong:
This looks like a related workaround in another package:
But for our # Directory containing the find modules relative to the config install
# directory.
file (RELATIVE_PATH glog_REL_CMake_MODULES
${_PREFIX}/${_glog_CMake_INSTALLDIR}
${_PREFIX}/${_glog_CMake_DATADIR}/glog-modules.cmake) |
I cannot pursue this further now. Somebody else needs to put some prints into the |
I think this is enough to fix it: --- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -868,7 +868,7 @@ configure_file (\"${CMAKE_CURRENT_SOURCE_DIR}/glog-modules.cmake.in\"
file (INSTALL
\"${CMAKE_CURRENT_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/CMakeTmp/glog-modules.cmake\"
DESTINATION
- \"\${CMAKE_INSTALL_PREFIX}/${_glog_CMake_INSTALLDIR}\")
+ \"${_glog_CMake_INSTALLDIR}\")
"
COMPONENT Development
) If a relative DESTINATION is given, it's assumed to be relative to the install prefix. So this is more correct anyway, and upstreamable as-is. |
@r-burns Great, that works:
Would you be up for making the upstream PR, since you understand better what's going on? |
I would rather not sign their CLA, especially for such a trivial change. |
@r-burns Makes sense. I will do it. |
Upstream PR: google/glog#733 |
Patch added to this PR. |
This is fixed now with the above changes. Still building, but 33 of 77 packages have built so far and none has failed. |
I get the same issue as google/glog#709 on x86_64-darwin, and it happens on master as well. Maybe best to exclude that test for now |
@r-burns OK, I disabled test on Darwin for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
@r-burns Would you be up for being co- |
Absolutely, feel free to add me |
Great, then we're done here. |
@GrahamcOfBorg build |
Upstream PR was merged. |
@r-burns FYI for the future, upstream
introduced a test failure for me in the nix build on Linux x86_64 (but not an Ubuntu build). Output:
This will be relevant when we upgrade to the next |
Motivation for this change
https://github.com/google/glog/releases/tag/v0.5.0
Things done
nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)