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
