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

[improve][doc] Simplify the Install section and promote a top-level "Installation" title #17580

Merged
merged 10 commits into from
Sep 16, 2022

Conversation

tisonkun
Copy link
Member

Fixes #17385

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@tisonkun tisonkun marked this pull request as ready for review September 10, 2022 16:54
@tisonkun
Copy link
Member Author

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 10, 2022
@tisonkun
Copy link
Member Author

Previews:

image

image

image

@tisonkun tisonkun changed the title Doc cpp client install [improve][doc] Simplify the Install section and promote a top-level "Installation" title Sep 10, 2022
@tisonkun
Copy link
Member Author

@merlimat @Anonymitaet Thanks for your reviews! Comments addressed.

Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

I have a concern that should we just link to https://github.com/apache/pulsar/tree/master/pulsar-client-cpp#compilation instead of adding repeated documents here?

Building C++ client from source is usually not easy for users other than experienced C++ developers. The content is a little complicated for basic users. They need to know:

  • The dependencies
  • How to install them
  • The requirements of the C++ compiler and CMake

And the details are all covered by https://github.com/apache/pulsar/tree/master/pulsar-client-cpp#readme

For example, I see you added -DBUILD_TESTS=OFF options. However, it's already covered in here, from which you can see other options like using DBUILD_PYTHON_WRAPPER=OFF to skip building python client.

Showing a simplified document might make those C++ users (not deep developers) confused and they might ask why cannot they build successfully after following the guide in the official website.

@tisonkun
Copy link
Member Author

I have a concern that should we just link to https://github.com/apache/pulsar/tree/master/pulsar-client-cpp#compilation instead of adding repeated documents here?

I agree that build from source has low frequency and users who want to do it will be OK to jump to the source code repository. Will push a commit tomorrow. Also, cc @merlimat @RobertIndie @shibd for inputs.

@tisonkun tisonkun force-pushed the doc-cpp-client-install branch from ecf6550 to b6df2f9 Compare September 14, 2022 02:10
Signed-off-by: tison <[email protected]>
@tisonkun
Copy link
Member Author

@shibd @Demogorgon314 updated. Here is the preview:

image

@tisonkun
Copy link
Member Author

tisonkun commented Sep 15, 2022

@BewareMyPower @Demogorgon314 @RobertIndie It seems we reached a consensus here. Could you help with merging this PR?

@Demogorgon314 Demogorgon314 merged commit 594c3e2 into apache:master Sep 16, 2022
@tisonkun tisonkun deleted the doc-cpp-client-install branch September 16, 2022 02:03
codertmy pushed a commit to codertmy/pulsar that referenced this pull request Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Doc][CPP] Simplify the Install section and promote a top-level "Install" title
8 participants