-
Notifications
You must be signed in to change notification settings - Fork 5
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
Modified the build.sh file to enable separate compilation of each module #5
base: main
Are you sure you want to change the base?
Modified the build.sh file to enable separate compilation of each module #5
Conversation
New menu options for easier modular testing and compilation
Add compile all option in brs_compile_component()
Add some comments to make debugging easily. Cc: dahogn <[email protected]> Cc: caiyuqing379 <[email protected]> Cc: meng-cz <[email protected]> Cc: USER0FISH <[email protected]> Signed-off-by: Zixuan Ding <[email protected]>
Hi, @qiaozhe1, please help reviewing this PR thanks |
Hi @hubblezzhu @qiaozhe1, During the actual compilation process, we have found some problems with the original build.sh script. So we modify build.sh to include options for compiling individual modules separately, which facilitates debugging. Could you please help reviewing it? Thanks. |
Can the option C be changed to automatically compile all components after waiting for a few seconds, hoping to minimize intervention after executing the script? |
Change the "C. Compile All Components" option to automatically start execution after a 10-second countdown. Signed-off-by: Zixuan Ding <[email protected]>
Hi @hubblezzhu @qiaozhe1, I modified brs_compile_component() in build.sh, changing the "C. Compile All Components" option to automatically start execution after a 10-second countdown. Could you please help reviewing it? Thanks. |
@vlsunil can you please check this PR ? |
build.sh
Outdated
break | ||
;; | ||
*) | ||
echo "Invalid option, please try again." |
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 am happy to see this enhancement which allows to build components selectively. However, I think we should have previous behavior as well. That is, if the user doesn't select any option within timeout value, then it should be do compile all, create images etc. What do you think?
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.
Ok, we will add this feature, wait for our update.
build.sh
Outdated
mmd -i $fatpart_name ::/EFI/BOOT/debug | ||
mmd -i $fatpart_name ::/EFI/BOOT/app | ||
|
||
mcopy -i $fatpart_name bootriscv64.efi ::/EFI/BOOT |
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.
Why these things are removed? With new changes I see only FAT FS creation. Who will setup files required in EFI/BOOT directory?
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.
Thank you for your feedback. This part was mistakenly deleted, but it has been corrected now. Could you please help reviewing it? Thanks.
Add time counting part in build.sh. Adjust brs_buildimage() in build.sh. Add brs-edk2-test-parser/Makefile so that it can be compiled. Signed-off-by:Zixuan Ding <[email protected]> Co-authored-by: USER0FISH <[email protected]> Cc: Sunil V L <[email protected]>
Remove the judgment in the secondary menu Signed-off-by:Signed-off-by:Zixuan Ding <[email protected]> Co-authored-by: USER0FISH <[email protected]> Cc: Sunil V L <[email protected]>
104005c
to
712e971
Compare
build.sh
Outdated
popd | ||
} | ||
popd | ||
} |
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.
Doesn't look necessary change. An extra space at the end is incorrect.
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.
Thank you for your reminder, we have deleted it in the latest commit.
build.sh
Outdated
local FAT2_SIZE_MB=128 | ||
|
||
# Assuming 2048 sectors for 1MB | ||
local PART_START=$((1 * 2048)) |
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.
Any reason to remove the macro BRS_SEC_PER_MB? I think it is better to keep the macro. Actually, apart from new lines introduced, I don't see any functional change. If so, better not to do such beautification as part of this PR since it takes review time unnecessarily.
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.
Thanks for reminding us, we have added BRS_SEC_PER_MB in the latest commit.
build.sh
Outdated
|
||
main | ||
# Start execute | ||
show_menu_with_countdown |
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.
Is there an extra new line at the end? github diff showing some kind of error.
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.
Yes, there should be a new line, and we have added it at the end.
build.sh
Outdated
cp $BRS_IMAGE_DIR/$BRS_IMAGE_XZ $TARGET_DIR/ | ||
|
||
cp "$BRS_IMAGE_DIR/$BRS_IMAGE_XZ" "$TARGET_DIR/" | ||
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.
extra spaces
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.
Thanks for reminding us, we have deleted these spaces in the latest commit.
src/brs-edk2-test-parser/Makefile
Outdated
.PHONY: brs-edk2-test-parser | ||
brs-edk2-test-parser: | ||
@echo "Building BRS EDK2-Test-Parser..." | ||
git clone -b $(EDK2_TEST_PARSER_VERSION) https://git.gitlab.arm.com/systemready/edk2-test-parser.git |
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.
Why do we use ARM repo? Please don't use sources from other architectures.
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.
We only found ARM repo, so we deleted this choice in our latest pull request.
Please confirm/provide testing results after the changes. |
OK, thanks for your feedback, we will fix these problems as soon as possible. |
Signed-off-by:Zixuan Ding <[email protected]> Co-authored-by: USER0FISH <[email protected]> Cc: Sunil V L <[email protected]>
Riscv brs tests v1.2
Modify build.sh to include options for compiling individual modules separately, which facilitates debugging.