At this point, GitLab launches a non-detached pipeline, but does so using
the SHA from before srccommit did its thing to update the VersionNum
(because
this is performed using a hook at the git level that GitLab knows nothing
about). The result is that the last artifacts presented on the MR page still
have the version numbers at their pre-increment state. It's still possible
to autobuild binaries with the new version number by manually launching a
pipeline afterwards, but that's an extra step for a human to perform and so
is undesirable.
To attempt to deal with this, introduce a check on the environment variable
CI_COMMIT_BRANCH
. This is only set when the pipeline was launched based on
a branch ref, and appears to include the case when an MR has just been
merged. If so, we assume the origin
remote contains a more up-to-date
version of this branch ref (i.e. one with VersionNum
updated) so we check
that out before commencing the rest of the build steps.
When launching a pipeline manually on a branch ref, this additional step should have no effect.
I definitely think it's worth testing this MR on an offline GitLab installation before merging it - can you help with this @jbyrne? It's just the sort of thing I can imagine GitLab screwing up, especially with a forked workflow, for example in cases where the same branch ref exists (pointing at different commits) in the fork project vs the upstream project.
ROOL (b801a772) at 26 Apr 12:55
When accepting a merge request, include VersionNum updates in binaries
ROOL (0b918ee9) at 22 Apr 09:13
Polish copyright job a bit
Skip files named .gitignore
wherever they are found. This is because they
are useful as placeholders to ensure otherwise empty directories are
created when checking out a revision. It's quite unlikely that any
non-trivial .gitignore
file would ever contain a copyright message.
Remove the leading ./
in the list of problematic files printed in the
case of job failure. This is for consistency with other jobs, which
increasingly omit it from filespecs they report.
This is achieved by using * .[!.]*
as the globs to start searching from.
Technically, this should be * .[!.]* ..?*
to match everything except .
and ..
, but in practice filenames beginning with two dots are extremely
rare, so this would normally result in ‘..?*’: No such file or directory
being printed to stderr. It's unlikely that either of the others will
produce such an error: there's always at least one file with one leading
dot (.gitlab-ci-yml
) and it's hard to imagine a useful repository
containing only files with one leading dot.
Leading ./
is still accepted in COPYRIGHT_WHITELIST
speifications for
compatibility reasons.
There's no harm done either way.
Well, quite. I was perhaps being overly cautious - we just need to be sure to check the binaries that get built during the first couple of MRs after this one is merged to be sure that nothing's been broken. Worst case is we back it out and manually re-run the affected pipelines, as you say.
Unmarked as draft
Aren't the two possible outcomes here
a) it works just fine
b) it doesn't work and you have to do the walk of shame and try again
There's no harm done either way. Alternatively, isn't it possible for ROOL to just manually run the job after the merge to pick up the right VersionNum, like has been done to date?
Unmarked as draft; it'll be important that this gets merged in advance of an upcoming RISC_OSLib change (so that the module version number is consistent with the version baked into the RMEnsure
statement in the stubs) and it seems there's no enthusiasm to test it on an offline GitLab instance.
Skip files named .gitignore
wherever they are found. This is because they
are useful as placeholders to ensure otherwise empty directories are
created when checking out a revision. It's quite unlikely that any
non-trivial .gitignore
file would ever contain a copyright message.
Remove the leading ./
in the list of problematic files printed in the
case of job failure. This is for consistency with other jobs, which
increasingly omit it from filespecs they report.
This is achieved by using * .[!.]*
as the globs to start searching from.
Technically, this should be * .[!.]* ..?*
to match everything except .
and ..
, but in practice filenames beginning with two dots are extremely
rare, so this would normally result in ‘..?*’: No such file or directory
being printed to stderr. It's unlikely that either of the others will
produce such an error: there's always at least one file with one leading
dot (.gitlab-ci-yml
) and it's hard to imagine a useful repository
containing only files with one leading dot.
Leading ./
is still accepted in COPYRIGHT_WHITELIST
speifications for
compatibility reasons.
ROOL (34712c81) at 01 Mar 08:57
Add a job to check usernames in commit logs
For auditing purposes, ROOL's policy is to require real names in commit logs (both author and committer), however there has been at least one example of a pseudonym slipping through the review net. GitLab doesn't make this any easier, because when displaying users in merge request pages, it looks up email addresses against those assigned to GitLab users, and substitutes the GitLab username instead. Only after the MR is accepted do you get to see the commits in pages where GitLab shows the underlying usernames from the git metadata, by which time it's too late to do anything about it!
This new CI job is an attempt to catch at least some future instances automatically. It iterates over the commits in an MR, and tests for one-word usernames (with the exception of "ROOL"), causing a job failure if any are found. All usernames are printed in full in the job log for human inspection.
Pedant
There's no real limit on the number of jobs, and one advantage of more, simpler jobs is that it offers finer granularity when disabling them. Having said that, I can't see an argument for disabling either (and for that matter, merge_log
shouldn't be allowed to fail, because if you get any tag settings wrong then the component will get mangled during merge acceptance) so I've grafted them together.
The word "nine" hasn't aged well.
Wouldn't this make more sense on the end of merge_log
? It relates to the log.
For auditing purposes, ROOL's policy is to require real names in commit logs (both author and committer), however there has been at least one example of a pseudonym slipping through the review net. GitLab doesn't make this any easier, because when displaying users in merge request pages, it looks up email addresses against those assigned to GitLab users, and substitutes the GitLab username instead. Only after the MR is accepted do you get to see the commits in pages where GitLab shows the underlying usernames from the git metadata, by which time it's too late to do anything about it!
This new CI job is an attempt to catch at least some future instances automatically. It iterates over the commits in an MR, and tests for one-word usernames (with the exception of "ROOL"), causing a job failure if any are found. All usernames are printed in full in the job log for human inspection.
These can be specified in the .gitlab-ci.yml
file (or if launching a
pipeline manually, via CI/CD
-> Pipelines
-> Run pipeline
) using the
CPPCHECK_EXTRA
variable. It is anticipated that this will be most useful
for specifying additional --suppress
arguments.
Because library components routinely feature functions that are not called
from elsewhere within the library itself, the unusedFunction
warning is
very commonly encountered in these components. To reduce the number of times
this needs to be specified in .gitlab-ci.yml
files, CPPCHECK_EXTRA
defaults to --suppress-unusedFunction
if it is unset for a library.
Because libraries are often listed as type C
in ModuleDB
, we instead
inspect the component name to see if it ends with Lib
in order to activate
this functionality.
To avoid merge conflicts, this MR is not based on master: you should merge !12 first.
This now uses a script installed on the Runner machine that performs a more
sophisticated analysis of the diagnostics generated by the underlying
cppcheck
tool. Recognising that many components already trigger multiple
cppcheck
diagnostics, this script effectively filters them out, to permit
us to focus on any new ones introduced in new work.
When a CI pipeline is launched on a branch or tag ref (including on push events, not limited to when an MR has been opened) this script normally analyses at least two commits (the one pointed at by the ref, and its first or only parent, unless there isn't one). In the common case that the commit is not yet on the default branch (or more specifically, when you're testing a fork project, on the default branch of the project it's forked from) then the script also analyses all commits back to their common ancestor. This usually corresponds to the set of commits that would be included in an MR.
For each commit, the number of each diagnostics of each type in each file is compared with those in the commit's first (or only) parent (if any). If the number increased, then information is printed to the job log. Diagnostics can be suppressed in a number of ways; if and only if the number of diagnostics that are not suppressed has also increased will the overall job status be considered a failure.
Suppressions use a wildcarded format, and can refer either to the diagnostic
name (as printed in red in the log), the filename, the file-and-line spec
(in the format <filename>:<line>
) or the file-and-diagnostic spec (in the
format <filename>:<diagnostic>
). Suppressions can be undone by the use of a
leading '-' character. Suppressions are space-separated and can be specified
by any of the following methods:
placing a line like this in a commit log (which only affects that commit):
Disable cppcheck for myfile.c:123.
placing a line like this in the variable declarations in .gitlab-ci.yml (this persists for all future commits, so it's best not to specify line numbers here):
CPPCHECK_WHITELIST: "myfile.c subdir/*.c portability/*"
assigning a value to CPPCHECK_WHITELIST
when launching a pipeline manually
Some diagnostics are also suppressed globally; the list of such suppressions is expected to be refined over time.
If you need more information to help understand why any diagnostic is being
triggered, it can be helpful to manually launch a pipeline with the
environment variable VERBOSE
set to 1.
Trial runs on actively-developed components indicate that in practice, it
will be rare that a commit that is ready for merging will need to suppress
any diagnostics. The most likely scenario in which one will be required is
in the event that a new submission uses a construct that triggers a "false
positive" in which cppcheck
incorrectly infers a fault. These are not
unheard of, but are typically fixed in a later revision of the tool. To
avoid the need to use inline suppression markers for these - which we would
typically want to remove at a later date when cppcheck
is upgraded - it
is therefore recommended that any such examples be dealt with using a
suppression in the commit log.
Obsoletes !13.
[Edit 2023-01-19: formatting only]
Hope that's the sort of thing you had in mind?
Perhaps it would benefit from a comment or two outlining what's going on?
There's a lot of documentation in the
compare-diagnostics.awk
file, which saves having to write it out twice, once for thewhitespace
job and once for thecppcheck
job.
I wasn't so much interested in how compare-diagnostics.awk
worked, more what the initial git diff-tree
and cppcheck --enable=all
commands were doing in order to be fed into the later diagnosis. I see you've put a whole bunch of extra comments in too which seem useful in themselves.
I'm going to dare to mention a spello in "Parantheses" though!
I just realised that one of the other improvements - the fact that I now explicitly add
-D__riscos
(and a couple of others) unconditionally - means that fuzz mode is always disabled anyway. So I've been able to simplify things by removing--max-configs=1 --suppress=toomanyconfigs
.
That sounds good, one problem solved and a simplified setup.