-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Scripts: refactor paddle bash shell scripts. #10038
Conversation
@typhoonzero @Yancey1989
|
FROM @luotao1
Sure. Thanks for @wanglei828's PR, could you create an issue to descript the background about this PR? |
paddle/scripts/paddle_build.sh
Outdated
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the indentation is not right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the indentation.
I have created a issue to describe my goal (#10073) |
Thanks to @wanglei828 for these very carefully developed scripts. I see duplicated code snippets in the two bash scripts in this PR. I'd suggest to merge them into one. Also, it would be great to update the documents, to add comments, or to add an issue, anyway, to describe the motivation and usage of the added script. Given that we currently have multiple scripts for building, it would be helpful to explain what existing scripts could be deleted once after this PR is merged. |
@wangkuiyi @luotao1 I have updated the "README.md" file to describe how to use the new scripts, please help review. Thanks! |
@wangkuiyi Duplicate functions have been merged into one. |
Thanks to @wanglei828 for this PR. I followed the issue and have some suggestions over there: #10073 (comment) |
Fix #10073