Re: [XEN PATCH] automation/eclair: Make report browsing URL configurable.

[Anthony PERARD <anthony@xxxxxxxxxxxxxx>]

On Thu, Jun 26, 2025 at 08:38:18AM +0200, Nicola Vetrini wrote:
> diff --git a/automation/eclair_analysis/ECLAIR/action.settings 
> b/automation/eclair_analysis/ECLAIR/action.settings
> index 1577368b613b..f822f0ea66d7 100644
> --- a/automation/eclair_analysis/ECLAIR/action.settings
> +++ b/automation/eclair_analysis/ECLAIR/action.settings
> @@ -14,9 +14,6 @@ autoPRRepository="${AUTO_PR_REPOSITORY:-}"
>  # Customized
>  autoPRBranch="${AUTO_PR_BRANCH:-}"
>  
> -# Customized
> -artifactsRoot=/var/local/eclair
> -
>  case "${ci}" in
>  github)
>      # To be customized
> @@ -166,12 +163,34 @@ esac
>  
>  ECLAIR_BIN_DIR=/opt/bugseng/eclair/bin/
>  
> -artifactsDir="${artifactsRoot}/xen-project.ecdf/${repository}/ECLAIR_${ANALYSIS_KIND}"
> +# Artifacts URL served by the eclair_report server
> +if [ -z "${MACHINE_ARTIFACTS_ROOT}" ];

You don't need a ';' if you have `then` on the next line ;-)

> +then
> +  echo "WARNING: No artifacts root supplied, using default"
> +fi
> +if [ -z "${MACHINE_ECDF_DIR}" ];
> +then
> +  echo "WARNING: No ecdf dir supplied, using default"
> +fi
> +artifactsRoot="${MACHINE_ARTIFACTS_ROOT:-/var/local/eclair}"
> +artifactsEcdfDir="${MACHINE_ECDF_DIR:-xen-project.ecdf}"

Do we need to separate varables for these two? It might be a bit simpler
to have:
    artifactsRoot=/var/local/eclair/xen-project.ecdf
unless there's other path than *.ecdf. But in any case, two separate
variables looks fine too.

> +artifactsDir="${artifactsRoot}/${artifactsEcdfDir}/${repository}/ECLAIR_${ANALYSIS_KIND}"
>  subDir="${subDir}${variantSubDir}"
>  jobHeadline="${jobHeadline}${variantHeadline}"
>  
> -# Customized
> -eclairReportUrlPrefix=https://saas.eclairit.com:3787
> +# Remote eclair_report hosting server
> +if [ -z "${MACHINE_HOST}" ];
> +then
> +  echo "WARNING: No machine host supplied, using default"
> +fi
> +if [ -z "${MACHINE_PORT}" ];
> +then
> +  echo "WARNING: No machine port supplied, using default"
> +fi
> +
> +eclairReportHost="${MACHINE_HOST:-saas.eclairit.com}"
> +eclairReportPort="${MACHINE_PORT:-3787}"
> +eclairReportUrlPrefix="https://${eclairReportHost}:${eclairReportPort}";

Please, don't make the port number mandatory. Can you merge both host
and port in the same variable? This part seems to be called "authority":

    https://en.wikipedia.org/wiki/URL#Syntax

Also, don't use `MACHINE` as prefix/namespace for these new variables,
in a pipeline context, "machine" could be many things. Maybe
"ECLAIR_REPORT_HOST" for this one? With the default been:

    ECLAIR_REPORT_HOST=saas.eclairit.com:3787

I wonder if "https" should be configurable as well, but I guess there
shouldn't be any need for it as we probably don't want to serve reports
over http.

>  
>  jobDir="${artifactsDir}/${subDir}/${jobId}"
>  updateLog="${analysisOutputDir}/update.log"
> diff --git a/automation/eclair_analysis/ECLAIR/action_push.sh 
> b/automation/eclair_analysis/ECLAIR/action_push.sh
> index 45215fbf005b..5002b48522e2 100755
> --- a/automation/eclair_analysis/ECLAIR/action_push.sh
> +++ b/automation/eclair_analysis/ECLAIR/action_push.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -set -eu
> +set -eux

Left over change from debugging?

>  
>  usage() {
>      echo "Usage: $0 WTOKEN ANALYSIS_OUTPUT_DIR" >&2
> diff --git a/automation/gitlab-ci/analyze.yaml 
> b/automation/gitlab-ci/analyze.yaml
> index 5b00b9f25ca6..f027c6bc90b1 100644
> --- a/automation/gitlab-ci/analyze.yaml
> +++ b/automation/gitlab-ci/analyze.yaml
> @@ -8,6 +8,8 @@
>      ENABLE_ECLAIR_BOT: "n"
>      AUTO_PR_BRANCH: "staging"
>      AUTO_PR_REPOSITORY: "xen-project/xen"
> +    MACHINE_ARTIFACTS_ROOT: "/space"
> +    MACHINE_ECDF_DIR: "XEN.ecdf"

Is this the right place for these variables? Shouldn't they be set on
gitlab (at project or repo scope) or even set by the runner itself.

>    script:
>      - ./automation/scripts/eclair 2>&1 | tee "${LOGFILE}"
>    artifacts:
> diff --git a/automation/scripts/eclair b/automation/scripts/eclair
> index 0a2353c20a92..7020eaa0982f 100755
> --- a/automation/scripts/eclair
> +++ b/automation/scripts/eclair
> @@ -1,4 +1,15 @@
> -#!/bin/sh -eu
> +#!/bin/sh -eux
> +
> +# Runner-specific variables
> +ex=0
> +export "$(env | grep MACHINE_ARTIFACTS_ROOT)" || ex=$?
> +[ "${ex}" = 0 ] || exit "${ex}"

That's a really complicated way to check a variable is set...
Exporting a variable that's already in env isn't useful, and I think
`ex` is only ever set to `0`. It seems that `dash` just exit if you do
`export=""`.

You could simply do:

    : ${MACHINE_ARTIFACTS_ROOT:?Missing MACHINE_ARTIFACTS_ROOT variable}
    : ${MACHINE_ECDF_DIR:?Missing MACHINE_ECDF_DIR variable}

To check that the variables are set. Or nothing, if you add `set -u` to
the script (instead of the one -u in the sheband which might be ignored
if one run `sh ./eclair` instead of just `./eclair`.) Also the variable
should come from the env, as nothing sets it, so no need to for that.

( The `:` at the begining of the line is necessary, and behave the same
way as `true` does. We need it because ${parm:?msg} is expanded.)

Or you could use `if [ -z "${param}" ]` if ${param:?msg} is too obscure.
We would just have "parameter not set" instead of a nicer message, due
to `set -u`.

Thanks,

-- 
Anthony PERARD