| Message ID | 20250326142754.5441-1-anthony.perard@vates.tech (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN] automation: Avoid changing source files for randconfig tests | expand |
On Wed, 26 Mar 2025, Anthony PERARD wrote: > We should avoid changing files from the source tree if we don't intend > to commit the result. > > We don't really need to check if $EXTRA_FIXED_RANDCONFIG is empty so > add it to the temporary file in all cases. > > Signed-off-by: Anthony PERARD <anthony.perard@vates.tech> > --- > automation/scripts/build | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/automation/scripts/build b/automation/scripts/build > index 522efe774e..8a3b8fb6b2 100755 > --- a/automation/scripts/build > +++ b/automation/scripts/build > @@ -12,12 +12,12 @@ $cc --version > # random config or default config > if [[ "${RANDCONFIG}" == "y" ]]; then > > - # Append job-specific fixed configuration > - if [[ -n "${EXTRA_FIXED_RANDCONFIG}" ]]; then > - echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/tools/kconfig/allrandom.config > - fi > + cp -f xen/tools/kconfig/allrandom.config xen/allrandom.config.tmp Wouldn't it be better to use mktemp? local tmpconfig=$(mktemp) cp -f xen/tools/kconfig/allrandom.config $tmpconfig > - make -j$(nproc) -C xen KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config randconfig > + # Append job-specific fixed configuration > + echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/allrandom.config.tmp > + > + make -j$(nproc) -C xen KCONFIG_ALLCONFIG=allrandom.config.tmp randconfig > > # RANDCONFIG implies HYPERVISOR_ONLY > HYPERVISOR_ONLY="y" > -- > > > Anthony Perard | Vates XCP-ng Developer > > XCP-ng & Xen Orchestra - Vates solutions > > web: https://vates.tech >
On Wed, Mar 26, 2025 at 07:10:52PM -0700, Stefano Stabellini wrote: > On Wed, 26 Mar 2025, Anthony PERARD wrote: > > diff --git a/automation/scripts/build b/automation/scripts/build > > index 522efe774e..8a3b8fb6b2 100755 > > --- a/automation/scripts/build > > +++ b/automation/scripts/build > > @@ -12,12 +12,12 @@ $cc --version > > # random config or default config > > if [[ "${RANDCONFIG}" == "y" ]]; then > > > > - # Append job-specific fixed configuration > > - if [[ -n "${EXTRA_FIXED_RANDCONFIG}" ]]; then > > - echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/tools/kconfig/allrandom.config > > - fi > > + cp -f xen/tools/kconfig/allrandom.config xen/allrandom.config.tmp > > Wouldn't it be better to use mktemp? > > local tmpconfig=$(mktemp) I though of it and I wasn't sure if we could use it in the CI, but it's already been used so that's an option. (Actually, there's only a single use by ./check-endbr.sh, ./configure does use it as well but to create temporary directory within the build tree.) But, to avoid overflowing /tmp with loads of leftover temporary files, we need to clean it, with: trap "rm $tmpconfig" EXIT The advantage of using an in-tree files with a predefined name is that it isn't going to create more than one file, no matter how many time you run ./build. The '*.tmp' files are already ignored by our .gitignore. I could rename it to with a "." to hide it a bit more. Thanks,
On Thu, 27 Mar 2025, Anthony PERARD wrote: > On Wed, Mar 26, 2025 at 07:10:52PM -0700, Stefano Stabellini wrote: > > On Wed, 26 Mar 2025, Anthony PERARD wrote: > > > diff --git a/automation/scripts/build b/automation/scripts/build > > > index 522efe774e..8a3b8fb6b2 100755 > > > --- a/automation/scripts/build > > > +++ b/automation/scripts/build > > > @@ -12,12 +12,12 @@ $cc --version > > > # random config or default config > > > if [[ "${RANDCONFIG}" == "y" ]]; then > > > > > > - # Append job-specific fixed configuration > > > - if [[ -n "${EXTRA_FIXED_RANDCONFIG}" ]]; then > > > - echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/tools/kconfig/allrandom.config > > > - fi > > > + cp -f xen/tools/kconfig/allrandom.config xen/allrandom.config.tmp > > > > Wouldn't it be better to use mktemp? > > > > local tmpconfig=$(mktemp) > > I though of it and I wasn't sure if we could use it in the CI, but it's > already been used so that's an option. (Actually, there's only a single > use by ./check-endbr.sh, ./configure does use it as well but to create > temporary directory within the build tree.) > > But, to avoid overflowing /tmp with loads of leftover temporary files, > we need to clean it, with: > > trap "rm $tmpconfig" EXIT > > The advantage of using an in-tree files with a predefined name is that > it isn't going to create more than one file, no matter how many time you > run ./build. The '*.tmp' files are already ignored by our .gitignore. I > could rename it to with a "." to hide it a bit more. I think it is fine, there is no need to hide them more. I was suggesting to create a file under /tmp instead to keep the source directory cleaner, and also because I don't think it is an issue to add files to /tmp and not clean them because they get removed when the container exits. Isn't it the case? Locally it looks like each containers gets its own /tmp that is cleaned after exit. So my preference is to use mktemp and *not* clean the tmp file on exit. If you think we have to clean the tmp file on exit, then let's go with your xen/allrandom.config.tmp approach as I would prefer to avoid the "trap" command to keep the sources simpler.
diff --git a/automation/scripts/build b/automation/scripts/build index 522efe774e..8a3b8fb6b2 100755 --- a/automation/scripts/build +++ b/automation/scripts/build @@ -12,12 +12,12 @@ $cc --version # random config or default config if [[ "${RANDCONFIG}" == "y" ]]; then - # Append job-specific fixed configuration - if [[ -n "${EXTRA_FIXED_RANDCONFIG}" ]]; then - echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/tools/kconfig/allrandom.config - fi + cp -f xen/tools/kconfig/allrandom.config xen/allrandom.config.tmp - make -j$(nproc) -C xen KCONFIG_ALLCONFIG=tools/kconfig/allrandom.config randconfig + # Append job-specific fixed configuration + echo "${EXTRA_FIXED_RANDCONFIG}" >> xen/allrandom.config.tmp + + make -j$(nproc) -C xen KCONFIG_ALLCONFIG=allrandom.config.tmp randconfig # RANDCONFIG implies HYPERVISOR_ONLY HYPERVISOR_ONLY="y"
We should avoid changing files from the source tree if we don't intend to commit the result. We don't really need to check if $EXTRA_FIXED_RANDCONFIG is empty so add it to the temporary file in all cases. Signed-off-by: Anthony PERARD <anthony.perard@vates.tech> --- automation/scripts/build | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)