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

add rbi-CDH: v1 #651

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

zny666
Copy link

@zny666 zny666 commented Aug 5, 2024

添加了基于镜像的构建,位于rbi-CDH文件夹,脚本在本地运行良好,目前缺少各种错误处理脚本。

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

PR is missing the DCO sign-off. Please add.

Also, you are changing the permissions of every file in the repo. I don't think that is necessary.

See additional comments below.

Comment on lines +8 to +9
sudo docker run -d --network host --name cdh-build1 rbi-cdh:v1
sudo docker run -d --network host --name cdh-build2 rbi-cdh:v1
Copy link
Member

@fitzthum fitzthum Aug 5, 2024

Choose a reason for hiding this comment

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

You seem to be running the same container image (rbi-cdh:v1) twice. Later you check that these contain the same CDH binary. Since they are the same container, they will contain the same binary. This isn't a reproducible build.

Copy link
Member

Choose a reason for hiding this comment

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

I think rbi-CDH is not the right place to put this. If you are adding an additional test, put it alongside the existing github workflows. If you are changing the makefile, try updating the existing makefile.

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.

2 participants