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

Runs make clean after make-install to reduce used disk #855

Merged
merged 5 commits into from
Apr 19, 2021

Conversation

chick
Copy link
Collaborator

@chick chick commented Apr 14, 2021

Make install leaves many .o and other files laying around
Fixes problems with disk space on Github Actions runner

Type of change: other enhancement

Impact: other
Reduces used disk space after toolchain build

Release Notes
Intermediate .o and other files are removed after building toolchain components.
Reduces disk requirements

Make install leaves many .o and other files
Fixes problems with disk space on github actions runner
@chick chick added the cleanup label Apr 14, 2021
@chick chick requested a review from abejgonzalez April 14, 2021 16:57
@chick chick self-assigned this Apr 14, 2021
Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

This LGTM, though we should quickly poll the Chipyard group if this is fine with them. Hopefully, there isn't a corner use case that wants the intermediate files.

@jerryz123
Copy link
Contributor

Lets make this a flag that CI sets.
It is useful to not run make clean if you are developing/debugging toolchain issues.

@abejgonzalez
Copy link
Contributor

abejgonzalez commented Apr 14, 2021

Lets make this a flag that CI sets.
It is useful to not run make clean if you are developing/debugging toolchain issues.

If the toolchain fails, then the intermediate files will not be deleted. The set -eo pipefail will catch it and prevent the clean from running. So the only way the clean will run is if the build passes.

@a0u
Copy link
Member

a0u commented Apr 14, 2021

For reclaiming space, it's better to deinit the toolchain submodules at the very end after the build succeeds.

@a0u
Copy link
Member

a0u commented Apr 14, 2021

I'd prefer having a flag to control this behavior. Cleaning after each step makes it harder to debug cases where an earlier step might succeed but was built with incorrect options, so the failure only manifests after attempting a later step that depends on the first. Retaining the intermediate files is also useful for incremental builds.

@abejgonzalez
Copy link
Contributor

abejgonzalez commented Apr 14, 2021

Sounds good, flag it is. This PR should probably also bump the CircleCI config.yml toolchain version so that the toolchain gets rebuilt (or have the hash depend on the build-toolchains and build-util.sh scripts).

@chick
Copy link
Collaborator Author

chick commented Apr 14, 2021

@abejgonzalez I've implemented a flag to keep the existing behavior the default. I'm not particularly enthused by the implementation. Feedback appreciated

-a | --arch )
shift
ARCH=$1 ;;
riscv-tools | esp-tools)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mis-aligned, the riscv-tools and ec2fast sections should be indented the same as the -a.

@abejgonzalez
Copy link
Contributor

I'm fine with this implementation (though I'm not picky at all about it). Can you bump the config.yml file as mentioned here:

Sounds good, flag it is. This PR should probably also bump the CircleCI config.yml toolchain version so that the toolchain gets rebuilt (or have the hash depend on the build-toolchains and build-util.sh scripts).

@@ -132,7 +136,7 @@ else
esac

module_prepare riscv-gnu-toolchain qemu
module_build riscv-gnu-toolchain --prefix="${RISCV}" --with-cmodel=medany ${ARCH:+--with-arch=${ARCH}}
module_build riscv-gnu-toolchain $CLEANAFTERINSTALL --prefix="${RISCV}" --with-cmodel=medany ${ARCH:+--with-arch=${ARCH}}
Copy link
Member

Choose a reason for hiding this comment

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

This only cleans the newlib toolchain build. To also clean after the Linux toolchain, the module_make function needs to be changed as well.

@@ -81,6 +86,9 @@ module_build() ( # <submodule> [configure-arg..]
"${MAKE}"
echo "==> Installing ${name}"
"${MAKE}" install
if [ "$clean_after_install" = "true" ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

Based on the original patch, I assumed the intent was to clean up after all the submodules rather than just riscv-gnu-toolchain:

Suggested change
if [ "$clean_after_install" = "true" ] ; then
if [ -n "$CLEANAFTERINSTALL" ] ; then

which makes parsing --clean-after-install unnecessary.

If a particular submodule needs to be excluded, the variable could be overridden at the call site:

CLEANAFTERINSTALL= module_build ...

chick added 2 commits April 14, 2021 17:30
…argument to `make_build` function.

Ups tools-cache-version to v7
… variable when using scripts

that call `module_make` or `module_build`. Those two bash functions will check CLEANAFTERINSTALL and if
it is non-empty, they will call `make clean` after other `make` calls.
@chick
Copy link
Collaborator Author

chick commented Apr 15, 2021

@a0u I changed this as you suggested, and I think it serves my purposes and preserves default behavior.
I don't want to clutter things but do you think I should add to the build-toolchains.sh usage to note its
sensitivity to the CLEANAFTERINSTALL env var?

@a0u
Copy link
Member

a0u commented Apr 15, 2021

@chick Sorry, I should have been more specific. I meant that build-toolchains.sh would still parse --clean-after-install and set CLEANAFTERINSTALL accordingly. module_build and module_make would just test whether this shell variable inherited from the outer subshell is non-empty, rather than parsing --clean-after-install a second time. This makes the behavior automatically consistent for all submodules using module_build; otherwise --clean-after-install would need to be explicitly passed at each call.

--clean-after-install to turn enable
`make clean` in `module_make` and `module_run`
@chick
Copy link
Collaborator Author

chick commented Apr 17, 2021

@a0u No problem, I think it's a lot clearer with it explicitly in the flags for build-toolchains.

@chick chick merged commit 70afeba into dev Apr 19, 2021
@jerryz123 jerryz123 deleted the call-make-clean-after-make-install branch October 1, 2022 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants