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

Update README.md #193

Merged
merged 2 commits into from
Oct 27, 2021
Merged

Update README.md #193

merged 2 commits into from
Oct 27, 2021

Conversation

gchatelet
Copy link
Collaborator

Change Quickstart section to match new default value for testing.

@gchatelet gchatelet requested a review from Mizux October 26, 2021 15:05
README.md Outdated
@@ -188,16 +188,16 @@ Please check the [CMake build instructions](cmake/README.md).

- build `list_cpu_features`
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

should use ```sh

@Mizux
Copy link
Collaborator

Mizux commented Oct 26, 2021

As general comment (open to debate !)

  1. I would move -H before -B aka tuple "source, build_dir" or "input, output" seems more natural to me.

  2. -H. is an hidden option maybe we should advertise -S. even if only in "recent" CMake.
    src: https://cmake.org/cmake/help/latest/release/3.13.html#command-line (introduction to -S. and official -B)
    src: https://cmake.org/cmake/help/latest/release/3.14.html#command-line (--verbose)

  3. Your command are Linux specific maybe we should try to provide a more neutral code block for our MacOS and Windows community.

My proposal (for 0,1,2):

cmake -S. -Bbuild -DCMAKE_BUILD_TYPE=Release
cmake --build build --config Release

note1: --config Release is needed for multi config generator (like Visual Studio, Xcode and ninja-multiconfig) and noop for make/ninja generator
note2: the default target all is OK otherwise you have --target ALL_BUILD for VS and XCODE and all on make/ninja IIRC
note3: technically -DCMAKE_BUILD_TYPE=Release is ignored for multi config generator and you should use -DCMAKE_CONFIGURATION_TYPES="Debug;Release" if you want to restrict the default list being Debug, Release,RelWithDebInfo and MinSizeRel
src: https://cmake.org/cmake/help/latest/variable/CMAKE_CONFIGURATION_TYPES.html

  1. Simply add a note about using -G to change the default generator ?
    note: if you use -G without argument cmake will fail with an "usage" message listing all available generator on your host system ;)
    e.g. on my gLinux (* is the default one, chosen by CMake)
%cmake -G
CMake Error: No generator specified for -G

Generators
  Green Hills MULTI            = Generates Green Hills MULTI files
                                 (experimental, work-in-progress).
* Unix Makefiles               = Generates standard UNIX makefiles.
  Ninja                        = Generates build.ninja files.
  Ninja Multi-Config           = Generates build-<Config>.ninja files.
  Watcom WMake                 = Generates Watcom WMake makefiles.
  CodeBlocks - Ninja           = Generates CodeBlocks project files.
  CodeBlocks - Unix Makefiles  = Generates CodeBlocks project files.
  CodeLite - Ninja             = Generates CodeLite project files.
  CodeLite - Unix Makefiles    = Generates CodeLite project files.
  Eclipse CDT4 - Ninja         = Generates Eclipse CDT 4.0 project files.
  Eclipse CDT4 - Unix Makefiles= Generates Eclipse CDT 4.0 project files.
  Kate - Ninja                 = Generates Kate project files.
  Kate - Unix Makefiles        = Generates Kate project files.
  Sublime Text 2 - Ninja       = Generates Sublime Text 2 project files.
  Sublime Text 2 - Unix Makefiles
                               = Generates Sublime Text 2 project files.
  1. For testing I would use
cmake --build build --config Release --target test

BUT with a note for windows/xcode user to use target RUN_TESTS (win) and RUN_TEST (xcode) respectively...

src:

@gchatelet
Copy link
Collaborator Author

Thx a lot for the suggestions.
I'll update the patch with a new version and we can discuss from here.

@gchatelet gchatelet marked this pull request as draft October 27, 2021 11:07
Change `Quickstart` section to match new default value for testing.
@gchatelet gchatelet requested a review from Mizux October 27, 2021 11:57
@gchatelet gchatelet marked this pull request as ready for review October 27, 2021 11:57
Copy link
Collaborator

@Mizux Mizux left a comment

Choose a reason for hiding this comment

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

LGTM

@gchatelet gchatelet merged commit 0dd8b41 into master Oct 27, 2021
@gchatelet gchatelet deleted the gchatelet-patch-1 branch October 27, 2021 12:48
@gchatelet gchatelet added this to the v0.7.0 milestone Mar 8, 2022
@gchatelet gchatelet added the misc non user facing: internal, cleanup, ci, release process label Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
misc non user facing: internal, cleanup, ci, release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants