.../eclair_analysis/ECLAIR/action.settings | 31 +++++++++++++++---- .../eclair_analysis/ECLAIR/action_push.sh | 2 +- automation/gitlab-ci/analyze.yaml | 2 ++ automation/scripts/eclair | 13 +++++++- 4 files changed, 40 insertions(+), 8 deletions(-)
Currently, the URL where the ECLAIR MISRA C scan reports are saved
is hardcoded; making it configurable allows multiple runners and storage
servers to be used without resorting to publishing all artifacts
to the same report server.
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Note: this is a key enabler for drafting more comprehensive community
analyses that can be possibly split between different machines
---
.../eclair_analysis/ECLAIR/action.settings | 31 +++++++++++++++----
.../eclair_analysis/ECLAIR/action_push.sh | 2 +-
automation/gitlab-ci/analyze.yaml | 2 ++
automation/scripts/eclair | 13 +++++++-
4 files changed, 40 insertions(+), 8 deletions(-)
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}" ];
+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}"
+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}"
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
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"
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}"
+export "$(env | grep MACHINE_ECDF_DIR)" || ex=$?
+[ "${ex}" = 0 ] || exit "${ex}"
+export "$(env | grep MACHINE_HOST)" || ex=$?
+[ "${ex}" = 0 ] || exit "${ex}"
+export "$(env | grep MACHINE_PORT)" || ex=$?
+[ "${ex}" = 0 ] || exit "${ex}"
ECLAIR_ANALYSIS_DIR=automation/eclair_analysis
ECLAIR_DIR="${ECLAIR_ANALYSIS_DIR}/ECLAIR"
--
2.43.0On 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
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
On Fri, Jun 27, 2025 at 10:39:21AM +0200, Nicola Vetrini wrote:
> On 2025-06-26 12:08, Anthony PERARD wrote:
> > On Thu, Jun 26, 2025 at 08:38:18AM +0200, Nicola Vetrini wrote:
> > > +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.
Well, the patch is all about "report browsing URL" as stated in the
subject.
> While we should alter the latter (e.g.,
> ECLAIR_REPORT_PROXY_HOST) to point to the proxy so that the proxy is shown
"proxy"? That's implementation detail, there's no need to know that the
host used to browse the result is different than the host where the
result are stored. (I mean having "proxy" in the name of the variable
holding detail of how to access the reports, been a part of an url or
just the host)
> 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.
Having the configuration for where to push the result in the runner env
seems like the best option to me, if that report server is tied to the
eclair runner.
> > > 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).
Isn't that script `./eclair` the beginning of the script? I can't find
anything that `source` this file so it must be. Which mean, if
MACHINE_ARTIFACTS_ROOT is set, it is exported, because nothing sets it
between the beginning of the script and this line. So there's no need to
check that the variable is exported (if it is not exported, it is a bug
in the shell).
The only thing left to do is to check that it is set and non-zero
length. But I still wonder why you do that here, since "action.settings"
also check the variable and use a default value.
There's no need to use bash for this. (They are plenty of reason to use
bash instead of posix shell, but that is just not one of them.)
Cheers,
--
Anthony PERARD
© 2016 - 2025 Red Hat, Inc.