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

build(cmake): simplify instructions (cmake -B build && cmake --build build ...) #6964

Merged
merged 10 commits into from
Apr 29, 2024

Conversation

ochafik
Copy link
Collaborator

@ochafik ochafik commented Apr 28, 2024

  • Updated instructions & CI to skip the mkdir -p build && cd build steps using config-time flag -B and build-time flag --build

    mkdir -b build
    cd build
    cmake ..
    make # or `cmake --build . --config Release`

    Becomes:

    cmake -B build
    cmake --build build --config Release
    # --config flag is ignored for Makefile generation (default) but crucial for Release builds w/ Xcode / MSVC, as pointed out by reviewers
  • Documented how to do Debug builds

  • Removed --config Release from most cmake build commands (except for Visual Studio) as Release is the default and the --config flag is often useless anyway)

    Note: To build in Debug w/ CMake one generally needs to set -DCMAKE_BUILD_TYPE=Debug in the "config" command (--config Debug on the build command works only for Ninja Multi-Config, XCode or Visual Studio generators)

@ochafik ochafik changed the title Simplify CMake build instructions (cmake . -B build && cmake --build build is all you need) build(cmake): simplify instructions (cmake . -B build && cmake --build build is all you need) Apr 28, 2024
README.md Outdated Show resolved Hide resolved
@cebtenzzre
Copy link
Collaborator

The source directory defaults to the current directory when using -B. So cmake . -B build should just be cmake -B build.

ochafik and others added 2 commits April 28, 2024 17:57
Co-authored-by: Jared Van Bortel <cebtenzzre@gmail.com>
@ochafik ochafik changed the title build(cmake): simplify instructions (cmake . -B build && cmake --build build is all you need) build(cmake): simplify instructions (cmake -B build && cmake --build build is all you need) Apr 28, 2024
@ochafik
Copy link
Collaborator Author

ochafik commented Apr 28, 2024

The source directory defaults to the current directory when using -B. So cmake . -B build should just be cmake -B build.

@cebtenzzre Neat! Updated.

@ochafik ochafik marked this pull request as ready for review April 28, 2024 17:14
README-sycl.md Outdated Show resolved Hide resolved
README-sycl.md Outdated Show resolved Hide resolved
Co-authored-by: Neo Zhang Jianyu <jianyu.zhang@intel.com>
@slaren
Copy link
Collaborator

slaren commented Apr 29, 2024

  • Removed --config Release from most cmake build commands (except for Visual Studio) as Release is the default and the --config flag is often useless anyway)

I am not sure that this is entirely correct. CMAKE_BUILD_TYPE configures the build type for single-config generators, while --config selects the build type for multi-config generators (ie. MSVC). I have been trying to find what is the default value of --config, but I have not been able to find any references. Are you sure that removing --config will still build in release with MSVC in all cases?

@slaren
Copy link
Collaborator

slaren commented Apr 29, 2024

I just ran a quick test, and without --config Release, it will result in a debug build. Therefore I think it is important to keep the --config Release, at least in the documentation that is meant to be applicable for all the platforms.

@ochafik
Copy link
Collaborator Author

ochafik commented Apr 29, 2024

Removed --config Release from most cmake build commands (except for Visual Studio) as Release is the default and the --config flag is often useless anyway)

I am not sure that this is entirely correct

@slaren Oops I misspoke, it's the default for single-generator configs (as set here), and the default is generator dependent for the others.

I just ran a quick test, and without --config Release, it will result in a debug build. Therefore I think it is important to keep the --config Release, at least in the documentation that is meant to be applicable for all the platforms.

Thanks a lot for catching this! I just checked if setting -DCMAKE_CONFIGURATION_TYPES=Release;Debug;MinSizeRel;RelWithDebInfo would give control over the default (hoping it'd pick the first) but it's not the case at least for Xcode.

I've reverted the --config Release bit and added instructions for Debug builds on the main README, PTAL 😁

@ochafik ochafik changed the title build(cmake): simplify instructions (cmake -B build && cmake --build build is all you need) build(cmake): simplify instructions (cmake -B build && cmake --build build ...) Apr 29, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@slaren slaren dismissed NeoZhangJianyu’s stale review April 29, 2024 15:59

Already addressed.

@ochafik ochafik merged commit b8a7a5a into ggerganov:master Apr 29, 2024
24 checks passed
nopperl pushed a commit to nopperl/llama.cpp that referenced this pull request May 5, 2024
… build ...`) (ggerganov#6964)

* readme: cmake . -B build && cmake --build build

* build: fix typo

Co-authored-by: Jared Van Bortel <cebtenzzre@gmail.com>

* build: drop implicit . from cmake config command

* build: remove another superfluous .

* build: update MinGW cmake commands

* Update README-sycl.md

Co-authored-by: Neo Zhang Jianyu <jianyu.zhang@intel.com>

* build: reinstate --config Release as not the default w/ some generators + document how to build Debug

* build: revert more --config Release

* build: nit / remove -H from cmake example

* build: reword debug instructions around single/multi config split

---------

Co-authored-by: Jared Van Bortel <cebtenzzre@gmail.com>
Co-authored-by: Neo Zhang Jianyu <jianyu.zhang@intel.com>
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.

5 participants