-
Notifications
You must be signed in to change notification settings - Fork 191
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
Improve the conan to build flow. #5148
Conversation
conanfile.py
Outdated
def generate(self): | ||
tc = CMakeToolchain(self) | ||
|
||
tc.cache_variables["BUILD_CLI"] = True | ||
tc.cache_variables["BUILD_RUBY_BINDINGS"] = True | ||
tc.cache_variables["BUILD_PYTHON_BINDINGS"] = True | ||
tc.cache_variables["BUILD_PYTHON_PIP_PACKAGE"] = False | ||
|
||
tc.cache_variables["BUILD_TESTING"] = bool(self.options.with_testing) | ||
tc.cache_variables["BUILD_BENCHMARK"] = bool(self.options.with_benchmark) | ||
|
||
tc.cache_variables["CPACK_BINARY_TGZ"] = True | ||
tc.cache_variables["CPACK_BINARY_IFW"] = False | ||
tc.cache_variables["CPACK_BINARY_DEB"] = False | ||
if self.settings.build_type == "Release": | ||
if is_apple_os(self) or self.settings.os == "Windows": | ||
tc.cache_variables["CPACK_BINARY_IFW"] = True | ||
else: | ||
tc.cache_variables["CPACK_BINARY_DEB"] = True | ||
tc.cache_variables["CPACK_BINARY_NSIS"] = False | ||
tc.cache_variables["CPACK_BINARY_RPM"] = False | ||
tc.cache_variables["CPACK_BINARY_STGZ"] = False | ||
tc.cache_variables["CPACK_BINARY_TBZ2"] = False | ||
tc.cache_variables["CPACK_BINARY_TXZ"] = False | ||
tc.cache_variables["CPACK_BINARY_TZ"] = False | ||
|
||
v = sys.version_info | ||
if (v.major, v.minor) == (3, 8): | ||
python_version = f"{v.major}.{v.minor}.{v.micro}" | ||
self.output.info(f"Setting PYTHON_VERSION and Python_ROOT_DIR from your current python: {python_version}, '{sys.base_prefix}'") | ||
tc.cache_variables["PYTHON_VERSION"] = python_version | ||
tc.cache_variables["Python_ROOT_DIR"] = Path(sys.base_prefix) | ||
else: | ||
self.output.warning( | ||
"Your current python is not in the 3.8.x range, which is what we target.\n" | ||
"You'll need to pass it properly when configuring CMake\n" | ||
"via -DPYTHON_VERSION:STRING='3.8.xx' and -DPython_ROOT_DIR:PATH='/path/to/python3.8/'" | ||
) | ||
tc.generate() |
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.
Define some common cacheVariables in the CMakePresets generated by conan, so
- You don't have to remember to set these properly if you use CMake Presets
- The conan install prints out a cmake command with these options should you use to manually run cmake configure
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.
The <build_dir>/CMakePesets.json will have
[...]
"configurePresets": [
{
"name": "conan-release",
"displayName": "'conan-release' config",
"description": "'conan-release' configure using 'Ninja' generator",
"generator": "Ninja",
"cacheVariables": {
"BUILD_CLI": "ON",
"BUILD_RUBY_BINDINGS": "ON",
[...]
BUILDING.md
Outdated
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.
Update the BUILDING.md accordingly, providing examples on how to configure & build with CMakePresets (recommend, and explained why), or manually. + a full example to make it clear
See it live at https://github.com/NREL/OpenStudio/blob/5fe2f5454cd07362f9f348a59999787704e2477e/BUILDING.md
CMakeLists.txt
Outdated
@@ -1,5 +1,5 @@ | |||
message(STATUS "Using CMake ${CMAKE_VERSION}") | |||
cmake_minimum_required(VERSION 3.20.0) | |||
cmake_minimum_required(VERSION 3.23.0) |
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.
Bump cmake min required to 3.23.0 so that CMake Presets are available.
v = sys.version_info | ||
if (v.major, v.minor) == (3, 8): | ||
python_version = f"{v.major}.{v.minor}.{v.micro}" | ||
self.output.info( | ||
f"Setting PYTHON_VERSION and Python_ROOT_DIR from your current python: {python_version}, '{sys.base_prefix}'" | ||
) | ||
tc.cache_variables["PYTHON_VERSION"] = python_version | ||
tc.cache_variables["Python_ROOT_DIR"] = Path(sys.base_prefix) | ||
else: | ||
self.output.warning( | ||
"Your current python is not in the 3.8.x range, which is what we target.\n" | ||
"You'll need to pass it properly when configuring CMake\n" | ||
"via -DPYTHON_VERSION:STRING='3.8.xx' and -DPython_ROOT_DIR:PATH='/path/to/python3.8/'" |
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.
Try to infer the Python info. If you use python 3.8, all is good, that'd be passed to cmake.
I'm using base_prefix and not exec_prefix or prefix, because I want a Virtualenv python to resolve to the underlying python (system python or pyenv python)
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.
Yeah, I don't think it will break the current setup(building in the external directory), and the one it offer will be more consistent. |
Oh ok, it broke the (ci)[https://ci.openstudio.net/blue/organizations/jenkins/openstudio-develop-full/detail/openstudio-develop-full/782/pipeline/128] pipeline .... let me fix it. |
Pull request overview
Improve the conan to build flow.
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.