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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2025-06-26 12:08, Anthony PERARD wrote:
> 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 ;-)
Hi Anthony,

yeah, missed that. Thanks

> > +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.
The main reason why I used two variables is that one may have 
differently-named .ecdf directories on different machines, but there is 
no strong reason have two variables
> > +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 can merge host and port and change the variable prefix, but I think 
there is a misunderstanding. This address is used both as the base for 
report browsing and pushing the results. While we should alter the 
latter (e.g., ECLAIR_REPORT_PROXY_HOST) to point to the proxy so that 
the proxy is shown in the CI logs, the address where the results are 
pushed is fixed and set in the docker runner env. This is not ideal, but 
I didn't find a better way with GitLab CI to let the analysis push 
locally.
> 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.
Yeah, I don't think so

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

> >  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.
Well, it was easier to set them there for debugging. The idea was to 
potentially override them at the runner config level, if needed.
> >    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`.
I agree it is ugly and counterintuitive, but the core idea here is that 
the variable is set but not exported for some reason, so just `export 
VAR` does not behave in the same way as the incantation `export "$(env | 
grep MACHINE_ARTIFACTS_ROOT)"` iirc. I'll double check if there's a 
better way to achieve this (other than switching to bash in the 
shebang).
Thanks,

--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253