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

doc: Improve cmake instructions in README #1641

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

fjahr
Copy link
Contributor

@fjahr fjahr commented Nov 26, 2024

Minor improvement suggestion for the readme. I find this alternative way of using cmake a bit more comfortable because I don't like to change the directory.

It's just a suggestion based on personal preference, if this is too minor of an improvement feel free to close.

@real-or-random
Copy link
Contributor

I happen to find this a tiny bit more comfortable, too, but IIRC we had discussed this (I can't find the discussion) and @hebasto argued that the tutorial cds into the build directory. I also think that mkdir build && cd build is a bit more self-documentary to people not familiar with CMake because it makes clear that build is a directory and not some CMake command or something.

No strong opinion from my side, but I think there are good reasons for what we have currently.

@fjahr
Copy link
Contributor Author

fjahr commented Nov 28, 2024

@real-or-random alright, thanks for the info, closing it for now

@fjahr fjahr closed this Nov 28, 2024
@hebasto
Copy link
Member

hebasto commented Nov 28, 2024

FWIW, while working on the CMake staging branch for Bitcoin Core, the rough consensus has been reached not to suggest using cd in the build docs (see https://github.com/bitcoin/bitcoin/blob/master/doc/build-*.md).

@fjahr
Copy link
Contributor Author

fjahr commented Dec 3, 2024

Ok, reopening since @real-or-random seemed in favor and I take from @hebasto 's comment that the argument for cd-ing seems not so relevant anymore and this would align the instructions here with bitcoin core.

@fjahr fjahr reopened this Dec 3, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@fjahr
Copy link
Contributor Author

fjahr commented Dec 3, 2024

Fixed comments from @hebasto, thanks!

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 93363fa

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

To unify all examples, -S . can be dropped from the example for Windows.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@fjahr
Copy link
Contributor Author

fjahr commented Dec 5, 2024

Thanks, took the suggested changes and removed the -S . from the windows example too.

@real-or-random
Copy link
Contributor

LGTM but sorry, a last nit: Perhaps then the comments for the autotools build should also be improved for consistency.

    $ ./autogen.sh       # Generate a ./configure script
    $ ./configure        # Generate a build system
    $ make               # Run the actual build process
    $ make check         # Run the test suite
    $ sudo make install  # Install the library into the system (optional)

And indenting to the same column is more readable, and then should also be done for the CMake instructions for consistency.

@fjahr
Copy link
Contributor Author

fjahr commented Dec 6, 2024

@real-or-random no worries, applied all the suggested changes

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 2ac9f55.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 2ac9f55

@real-or-random real-or-random merged commit f79f46c into bitcoin-core:master Dec 9, 2024
41 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build user-documentation user-facing documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants