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

CMake args tracking in exe and installations #2210

Merged
merged 4 commits into from Sep 19, 2018

Conversation

Projects
None yet
3 participants
@bilke
Copy link
Member

bilke commented Sep 18, 2018

Please review commit-wise.

@bilke bilke added the please review label Sep 18, 2018

@bilke bilke force-pushed the bilke:cmake-args branch 2 times, most recently from f66a513 to aef8392 Sep 18, 2018

@@ -1,14 +1,22 @@
# Disallow in-source builds

This comment has been minimized.

@endJunction

endJunction Sep 18, 2018

Member

Why?
Maybe adding the specific reasons to this comment would be good...

This comment has been minimized.

@bilke

bilke Sep 18, 2018

Author Member

It disallows running cmake in the source root directory only as this clutters your git project. You can of course still have source/build or similar. build*/ is even git-ignored.

@endJunction
Copy link
Member

endJunction left a comment

Much cleaner!

I didn't understand the details for the cached variables which do work on the first run but not on a subsequent one... confused.

@chleh

chleh approved these changes Sep 18, 2018

Copy link
Collaborator

chleh left a comment

Looks good.

@@ -1,14 +1,22 @@
# Disallow in-source builds
if("${PROJECT_SOURCE_DIR}" STREQUAL "${PROJECT_BINARY_DIR}")

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

Wouldn't a more general check, e.g. if ("${PROJECT_BINARY_DIR}/" STRING_STARTSWITH "${PROJECT_SOURCE_DIR}") be even better. (the search operator is probably not defined.)

This comment has been minimized.

@bilke

bilke Sep 18, 2018

Author Member

But this would also exclude source/build/.. which I find handy. See also comment above.

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

OK

@@ -23,6 +23,7 @@ endif()
project( OGS-6 )

include(scripts/cmake/CMakeSetup.cmake)
include(ParseCMakeArgs)

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

no "scripts/cmake/" needed?

@@ -0,0 +1,19 @@
# Captures not-yet cached CMake variables.
# On first CMake run via cmake-cli this works as expected.
# Once the variables are cached this will not work anymore.

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

... and the use-case for this is overriding certain settings...?

This comment has been minimized.

@bilke

bilke Sep 18, 2018

Author Member

No I just want to have the info on the CMake configuration inside the OGS exe. As I said this only works if you specify everything on the first CMake run (as we do in Jenkins, so the Jenkins provided binaries contain the correct information).

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

Sounds reasonable and will probably simplify bug hunting in our distributed binaries.

@@ -126,3 +126,6 @@ endif()

configure_file(Documentation/README.txt.in ${PROJECT_BINARY_DIR}/README.txt)
install(FILES ${PROJECT_BINARY_DIR}/README.txt DESTINATION .)

install(FILES ${PROJECT_BINARY_DIR}/CMakeCache.txt DESTINATION ${CMAKE_INSTALL_INFODIR})

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

For reproducible builds? Nice.

This comment has been minimized.

@bilke

bilke Sep 18, 2018

Author Member

Not for reproducing but for checking which config one actually has..

BaseLib::BuildInfo::git_describe,
BaseLib::BuildInfo::git_describe + "\n" +
"CMake arguments: " +
BaseLib::BuildInfo::cmake_args,

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

As far as I understand, in the past some user/developer reported/experienced some problems caused by manually set cmake variables, which now are reported by ogs --help. That looks like nice feature.

Maybe also some people have problems caused by "dirty" source directories. Therefore (different PR) it might also be helpful to write a status message like "Warning: Source dir dirty, there were changes in the following files: ..." to the screen (and maybe also to each vtu file that we write).

This comment has been minimized.

@endJunction

endJunction Sep 18, 2018

Member

the '-dirty' postfix is attached by git describe in that case...

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

Ah, OK.

@@ -32,6 +32,7 @@ namespace BuildInfo

extern BASELIB_EXPORT const std::string git_describe;
extern BASELIB_EXPORT const std::string ogs_version;
extern BASELIB_EXPORT const std::string cmake_args;

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

Btw. BASELIB_EXPORT seems to be an empty macro. Can't we just remove it?

This comment has been minimized.

@bilke

bilke Sep 18, 2018

Author Member

I thought this is required for shared libs on Windows (.dlls)?

This comment has been minimized.

@chleh

chleh Sep 18, 2018

Collaborator

Yes, probably. I got confused. After some googling I now roughly understand how this works.
https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html

@bilke bilke force-pushed the bilke:cmake-args branch from aef8392 to 919e4ae Sep 18, 2018

@bilke bilke added don't merge and removed please review labels Sep 18, 2018

@bilke

This comment has been minimized.

Copy link
Member Author

bilke commented Sep 18, 2018

@endJunction Don't merge I will explain a bit more..

bilke added some commits Sep 18, 2018

[CMake] Added ParseCMakeArgs.cmake
Stores user-given CMake variables from the recent CMake run into cmake-args file. Install cmake-args file and CMakeCache.txt.

@bilke bilke force-pushed the bilke:cmake-args branch from 919e4ae to f6c4ccf Sep 19, 2018

@bilke bilke removed the don't merge label Sep 19, 2018

@bilke bilke merged commit dfca0f8 into ufz:master Sep 19, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
deploy/netlify Deploy preview ready!
Details

@bilke bilke deleted the bilke:cmake-args branch Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.