Upgrade build system #75
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 54c7c84 in 1 minute and 1 seconds
More details
- Looked at
1108lines of code in43files - Skipped
2files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. .gitmodules:1
- Draft comment:
The removal of submodules from .gitmodules should be reflected in the removal of their usage in the codebase. Ensure that any references to these submodules in the codebase are also removed or updated accordingly. - Reason this comment was not posted:
Comment did not seem useful.
2. CMakeLists.txt:26
- Draft comment:
Useif(SC_BUILD_TESTS)instead ofif(${SC_BUILD_TESTS})to check the option correctly. This applies to other similar conditions in this file. - Reason this comment was not posted:
Confidence changes required:50%
The CMakeLists.txt file has been updated to use modern CMake practices, which is good. However, the use ofif(${SC_BUILD_TESTS})andif(${SC_CLANG_FORMAT_CODE})should beif(SC_BUILD_TESTS)andif(SC_CLANG_FORMAT_CODE)respectively, as these are options and should be checked without the${}.
3. CMakePresets.json:3
- Draft comment:
Ensure that the includedConanPresets.jsonfile exists and is correctly configured to avoid build issues. - Reason this comment was not posted:
Confidence changes required:50%
TheCMakePresets.jsonfile is well-structured and includes necessary configurations for different build types. However, it is important to ensure that the includedConanPresets.jsonfile exists and is correctly configured to avoid build issues.
4. cmake/ClangFormat.cmake:17
- Draft comment:
Ensure that theclang-formattool is available in the environment to avoid configuration errors when this script is executed. - Reason this comment was not posted:
Confidence changes required:50%
TheClangFormat.cmakefile is well-structured and provides a good setup for formatting targets. However, ensure that theclang-formattool is available in the environment where this CMake script is executed to avoid configuration errors.
5. cmake/install.cmake:4
- Draft comment:
Ensure that all paths and variables used in this script are correctly defined and available in the build environment to avoid installation issues. - Reason this comment was not posted:
Confidence changes required:50%
Theinstall.cmakescript is well-structured for installing targets and setting up CPack. However, ensure that all paths and variables used are correctly defined and available in the build environment to avoid installation issues.
6. cmake/tests.cmake:17
- Draft comment:
Ensure that theGTestlibrary is correctly linked and available in the build environment to avoid test execution issues. - Reason this comment was not posted:
Confidence changes required:50%
Thetests.cmakefunction is well-structured for creating tests from a folder. However, ensure that theGTestlibrary is correctly linked and available in the build environment to avoid test execution issues.
7. conanfile.py:13
- Draft comment:
Ensure that the specified versions of dependencies are available in the Conan repository to avoid build issues. - Reason this comment was not posted:
Confidence changes required:50%
Theconanfile.pyscript is well-structured for defining Conan dependencies. However, ensure that the specified versions of dependencies are available in the Conan repository to avoid build issues.
8. ostis-metasystem.ini:9
- Draft comment:
Ensure that the removal of theextensionspath is intentional and that the system can function correctly without this configuration. - Reason this comment was not posted:
Confidence changes required:50%
Theostis-metasystem.inifile has been updated to remove theextensionspath. Ensure that this change is intentional and that the system can function correctly without this configuration.
Workflow ID: wflow_DQ2WpX6YdwcQwizd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
NikitaZotov
force-pushed
the
feat/update_build_system
branch
from
46548bd to
eeb3562
Compare
|
@ellipsis, review PR, please |
|
I attempted to review the pull request, but it seems there is an issue accessing it. Please verify the pull request's status or visibility, and let me know if there are any changes. |
|
@ellipsis, give me a code review |
|
I'm currently unable to access PR #75 for a review. Please verify the repository's access permissions or try again later. If the issue persists, consider reaching out for support. |
kilativ-dotcom
requested changes
Feb 3, 2025
sc-models/knowledge-base/ostis-metasystem-components-specification.scs
Outdated
Show resolved
Hide resolved
| 2. Install C++ problem solver: | ||
|
|
||
| ```sh | ||
| ./scripts/install_cxx_problem_solver.sh |
There was a problem hiding this comment.
need to check this after everything is released
There was a problem hiding this comment.
so error here is expected currently?
There was a problem hiding this comment.
Yes, not all repos are release yet
...em-solver/cxx/identifiers-module/agent/translate_main_system_idtfs_from_sc_to_file_agent.cpp
Show resolved
Hide resolved
| 2. Install C++ problem solver: | ||
|
|
||
| ```sh | ||
| ./scripts/install_cxx_problem_solver.sh |
There was a problem hiding this comment.
so error here is expected currently?
KovalM
approved these changes
Feb 4, 2025
|
|
||
| ### Pull Request Review | ||
|
|
||
| - Reviewer should test code from PR if CI don't do it; |
There was a problem hiding this comment.
CI is something that happens in the past relative to the testing by reviewer so it should use some past tense
| To check code with CLangFormat run: | ||
| ```sh | ||
| cmake --preset release-with-tests-conan -DSC_CLANG_FORMAT_CODE=ON | ||
| cmake --build --preset release --target clangformat_check |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on:
Important
Upgrade build system by updating CMake configuration, adding Conan, and modifying scripts and logging.
CMakeLists.txtto require CMake version 3.24 and set project version to 0.1.0.CMakePresets.jsonfor build configuration presets.conanfile.py.ostis-web-platformandthirdparty/googletestfrom.gitmodules.cmake/ClangFormat.cmakefor code formatting.cmake/install.cmakefor installation configuration.cmake/tests.cmakefor test configuration.scripts/clang/check_formatting.shandscripts/clang/format_code.shfor code formatting.scripts/install_metasystem.shto remove platform installation steps.scripts/set_vars.shto remove platform-related variables.translate_main_system_idtfs_from_sc_to_file_agent.cppand other agents to usem_logger.ostis-metasystem.inito changeparallel_actionsto 1.identifiers-moduleandsections-modulein respectivetestdirectories.This description was created by
for 54c7c84. It will automatically update as commits are pushed.