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

[clang][docs] Improve "Obtaining Clang" section #71313

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

robincaloudis
Copy link
Contributor

@robincaloudis robincaloudis commented Nov 5, 2023

Why

The documentation is written relatively to clang-llvm, not the root repository directory. However, some steps in the documentation are relative to the repository root, which is not correct.

What

Documentation steps have been modified to make them correct and outdated ones were updated. Some details:

  • Correct paths in documentation
  • Change bootstrap.py -> configure.py since bootstraping Ninja has slightly changed

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Nov 5, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 5, 2023

@llvm/pr-subscribers-clang

Author: Robin Caloudis (robincaloudis)

Changes

The documentation is written relatively to clang-llvm, not the root repository directory. Therefore, the llvm-project repo needs to be cloned into the existing directory clang-llvm. As cloning into an existing directory is only allowed if the directory is empty, I added mkdir ~/clang-llvm to make the intent of creating an empty directory explicit.


Full diff: https://github.com/llvm/llvm-project/pull/71313.diff

1 Files Affected:

  • (modified) clang/docs/LibASTMatchersTutorial.rst (+2-2)
diff --git a/clang/docs/LibASTMatchersTutorial.rst b/clang/docs/LibASTMatchersTutorial.rst
index 37c9f178fa8df31..2a3f052f2e9ce15 100644
--- a/clang/docs/LibASTMatchersTutorial.rst
+++ b/clang/docs/LibASTMatchersTutorial.rst
@@ -22,8 +22,8 @@ started guide <https://llvm.org/docs/GettingStarted.html>`_.
 
 .. code-block:: console
 
-      cd ~/clang-llvm
-      git clone https://github.com/llvm/llvm-project.git
+      mkdir ~/clang-llvm && cd ~/clang-llvm
+      git clone https://github.com/llvm/llvm-project.git .
 
 Next you need to obtain the CMake build system and Ninja build tool.
 

@robincaloudis robincaloudis changed the title [clang][docs] Improve "Obtainig Clang" section [clang][docs] Improve "Obtaining Clang" section Nov 5, 2023
@robincaloudis
Copy link
Contributor Author

@r4nt, could you please review? Thank you.

@robincaloudis
Copy link
Contributor Author

Ping.

@robincaloudis
Copy link
Contributor Author

Ping

@Endilll
Copy link
Contributor

Endilll commented Jan 15, 2024

LGTM, but wair for other reviewers.

cd ~/clang-llvm
git clone https://github.com/llvm/llvm-project.git
mkdir ~/clang-llvm && cd ~/clang-llvm
git clone https://github.com/llvm/llvm-project.git .
Copy link
Collaborator

@jrtc27 jrtc27 Jan 15, 2024

Choose a reason for hiding this comment

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

Given the:

      cd ~/clang-llvm
      git clone git://cmake.org/stage/cmake.git

(which will clash with the cmake/ in llvm-project's root) this isn't a good idea. Better to clone it to an llvm-project subdirectory (i.e. drop the .) and alter later steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @jrtc27. Indeed, having the llvm-project subdirectory is much cleaner. I updated all required steps. Found some more outdated details. Updated PR description. Please re-review. Thank you.

The documentation is written relatively to
`clang-llvm`, not the root repository directory.
Therefore, the `llvm-project` repo needs to be
cloned into the existing directory `clang-llvm`.
I added `mkdir ~/clang-llvm` to make the intent
of creating an empty directory explicit.
Furthermore, additional steps have been modified
accordingly.
Copy link
Collaborator

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks better now

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 16, 2024

Though please make sure to update the PR message (which will become the commit message) before merging, as it's no longer accurate

@robincaloudis
Copy link
Contributor Author

robincaloudis commented Jan 16, 2024

Though please make sure to update the PR message (which will become the commit message) before merging, as it's no longer accurate

Thanks. Updated it.

@robincaloudis
Copy link
Contributor Author

@jrtc27, I do not have write access. All checks passed. Can you merge the PR? Thanks.

@Endilll Endilll merged commit e8af89e into llvm:main Jan 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants