[packages] add clang-format #2999
Conversation
github-actions
bot
added
scripts
There was a problem hiding this comment.
Hi @amin1377 , this change makes sense to me. Happy that we are updating the container.
Just a minor comment about adding a comment.
|
|
||
| # Required for code formatting | ||
| sudo apt-get install -y \ | ||
| clang-format-18 |
There was a problem hiding this comment.
We mention that install_apt_packages.sh works with Ubuntu 18, 20, 22 and 24 in the quick start docs, but I believe versions 18 to 22 do not have clang-format-18 in the default apt repos. You should either change the docs for this and remove support for the older versions, or alternatively add an if/else statement to the script to just not install clang-format-18 for the Ubuntu versions that don't have clang-format-18 and say that make format is only supported for Ubuntu 24.04 or newer in the docs. This way people on older Ubuntus can at least compile and use VTR with the same script, they just can't use make format.
You can use this command to check if clang-format-18 is available (it actually checks the local apt-cache but since we do a apt-get update in the script it should be up to date and reliable):
apt-cache search --names-only 'clang-format-18'
There was a problem hiding this comment.
Good point, thanks! I added the condition.
There was a problem hiding this comment.
After discussing with @AmirhosseinPoolad , we decided to update the documentation to inform users that they may encounter issues on Ubuntu versions older than 24.04. We will also include instructions on how to resolve these issues.
doc/src/quickstart/index.rst
Outdated
|
|
||
| VTR requires several system and Python packages to build and run the flow. | ||
| Ubuntu users can install the required system packages using the provided script or | ||
| the command below. This setup works on Ubuntu 18.04, 20.04, 22.04, and 24.04, but note |
There was a problem hiding this comment.
Minor nitpick: The way you phrase this makes it sound like you MUST have clang-format installed in order to use VTR. This may put off some people. I think you should add a sentence mentioning that clang-format is optional and used only for development of VTR.
There was a problem hiding this comment.
Changed the line about installing clang-format-18 to the following:
To install ``clang-format-18`` on older Ubuntu versions (e.g., 20.04 or 22.04), you must add the LLVM repository manually. Note that this tool is only required if you want to run ``make format`` to automatically fix formatting issues in the code. It is not necessary for building or running VPR.
Add clang-format-18 to the list of required packages