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 update #156

Merged
merged 22 commits into from
Oct 26, 2023
Merged

Cmake update #156

merged 22 commits into from
Oct 26, 2023

Conversation

sameeul
Copy link
Member

@sameeul sameeul commented Oct 24, 2023

  • Update CMakeList.txt and tests/CMakeLists.txt to streamline dependencies
  • Update prereq installation scripts. Now takes a --min_build yes option to install only the minimal deps.
  • Update GitHub Action workflows
  • Fix ReadTheDocs build
  • Update "Build from source" instructions.

@sweep-ai
Copy link

sweep-ai bot commented Oct 24, 2023

Apply Sweep Rules to your PR?

  • Apply: Leftover TODOs in the code should be handled.
  • Apply: All new business logic should have corresponding unit tests in the tests/ directory.
  • Apply: Any clearly inefficient or repeated code should be optimized or refactored.

CMakeLists.txt Outdated
set(USE_ARROW OFF)
endif()

if(BUILD_WITH_ALL)
Copy link
Member

@friskluft friskluft Oct 26, 2023

Choose a reason for hiding this comment

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

Great PR overall. The only thing that I'd change/improve is the naming "MINIMUM_BUILD - BUILD_WITH_ALL". To me, "BUILD_WITH_ALL" sounds rather non-intuitive and not self-explanatory (Build with all the =what=?) Secondly, the pair "MINIMUM_BUILD - BUILD_WITH_ALL" is non-symmetric morphologically. Thirdly, I would stick to underscore-free flag convention as it reduces guessing whether words are separated or not. The naming natural to me would be "MINBUILD - MAXBUILD" or "NOEXTRAS - ALLEXTRAS" or so, but it's only my subjective suggestion.

@sameeul sameeul merged commit a11ac46 into PolusAI:main Oct 26, 2023
18 checks passed
@sameeul sameeul deleted the cmake_update branch October 31, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants