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

【code format check upgrade】 step2:cmake-format #43057

Merged
merged 2 commits into from
Jun 4, 2022

Conversation

betterpig
Copy link
Contributor

@betterpig betterpig commented May 27, 2022

PR types

Others

PR changes

Others

Describe

add cmake-format to help format cmake files.
some points:

  • wont format comment, because most comment are designed carefully and already have a good style. Only few of them has problem of line width over limit, which is easy to fix manully.
  • use .cmake-format.py as config file, and add some pattens to tell cmake-format how to format the self-defining function such as cc_library, which can make the code more elegant and easy to read. For example:

before:
image

after:
image

  • exclude one file paddle/fluid/operators/CMakeLists.txt, because tools/remove_grad_op_and_kernel.py need to update it, and errors occured after format this file, so just let it keep original format.

@paddle-bot-old
Copy link

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@betterpig betterpig changed the title 【no merge】cmake-format results 【code format check upgrade】 step2:cmake-format Jun 2, 2022
- id: cmake-format
exclude: |
(?x)^(
paddle/fluid/operators/CMakeLists.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

请问这个exclude是有什么考虑么?后面会移走么?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

该文件格式化之后inference流水线报错,原因为tools/remove_grad_op_and_kernel.py需要对该文件进行修改,格式化后读取该文件出错。
报错截图:
image

tools/remove_grad_op_and_kernel.py代码截图:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

明白了,remove_grad_op_and_kernel.py 是推理用来裁剪反向Op的,可以后续看下如何格式化

CMakeLists.txt Outdated
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations under
# the License
Copy link
Contributor

Choose a reason for hiding this comment

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

对 License的修改有点问题。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

考虑到注释一般都是已经排好版的,只要极少数注释会具有列宽超限问题,因此决定不对注释进行格式化,对于注释的把控主要靠开发同学自身,以及cmakelint对于列宽超限提示的辅助。
已通过在.cmake-format.py里设置 enable_markup = False 屏蔽掉对注释的格式化。

# CBLAS_INC_DIR # the include directory for cblas.
# CBLAS_LIBS # a list of libraries should be linked by paddle.
# # Each library should be full path to object file.
# CBLAS_PROVIDER # one of MKLML, OPENBLAS, REFERENCE CBLAS_INC_DIR # the
Copy link
Contributor

Choose a reason for hiding this comment

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

这儿注释部分也乱了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

见上一条评论。

Copy link
Contributor

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1
Copy link
Contributor

luotao1 commented Jun 4, 2022

2022-06-03 11:39:23 3. You must be approved by chenwhql or zyfncg for paddle/phi/kernels/declarations.h using. Thanks!
https://xly.bce.baidu.com/paddlepaddle/paddle/newipipe/detail/5776637/job/15453992
@chenwhql 请问这个PR是怎么触发这个规则的呢,要如何修复?

@luotao1 luotao1 merged commit 92568ed into PaddlePaddle:develop Jun 4, 2022
fuyou765 pushed a commit to fuyou765/Paddle that referenced this pull request Jun 7, 2022
@betterpig betterpig deleted the cmake_format branch June 8, 2022 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants