[XEN PATCH] automation: Avoid changing source files for randconfig tests

[XEN PATCH] automation: Avoid changing source files for randconfig tests

[Anthony PERARD]

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
 
-    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

Re: [XEN PATCH] automation: Avoid changing source files for randconfig tests

[Stefano Stabellini]

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
> 

Re: [XEN PATCH] automation: Avoid changing source files for randconfig tests

[Anthony PERARD]

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,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH] automation: Avoid changing source files for randconfig tests

[Stefano Stabellini]

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.

Re: [XEN PATCH] automation: Avoid changing source files for randconfig tests

[Anthony PERARD]

On Thu, Mar 27, 2025 at 03:59:16PM -0700, Stefano Stabellini wrote:
> I was suggesting to create a file under /tmp instead to keep the source
> directory cleaner,

There's an easy way to keep the source directory *extra clean* when
doing hypervisor build, like we are doing here for randoconfig, that is:
out-of-tree build!

> 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.

Sorry, I tend to think that those script could be use outside of the CI
or containers, and /tmp can have a lot of different configuration, so
it's better to clean after oneself when possible.

As for running containers locally, maybe it's faster to reuse a
container instead of creating a new one from an image, which mean /tmp
is probably not cleaned on exit. But I guess people usuasly do `docker
run --rm` or use `containerize` which does the same.

> 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.

So, is that an Ack on my original patch?

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [XEN PATCH] automation: Avoid changing source files for randconfig tests

[Stefano Stabellini]

On Fri, 4 Apr 2025, Anthony PERARD wrote:
> On Thu, Mar 27, 2025 at 03:59:16PM -0700, Stefano Stabellini wrote:
> > I was suggesting to create a file under /tmp instead to keep the source
> > directory cleaner,
> 
> There's an easy way to keep the source directory *extra clean* when
> doing hypervisor build, like we are doing here for randoconfig, that is:
> out-of-tree build!
> 
> > 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.
> 
> Sorry, I tend to think that those script could be use outside of the CI
> or containers, and /tmp can have a lot of different configuration, so
> it's better to clean after oneself when possible.
> 
> As for running containers locally, maybe it's faster to reuse a
> container instead of creating a new one from an image, which mean /tmp
> is probably not cleaned on exit. But I guess people usuasly do `docker
> run --rm` or use `containerize` which does the same.
> 
> > 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.
> 
> So, is that an Ack on my original patch?

Yes