| Message ID | ae1e35ca4f0557873fa25ca5205d92aa2a16dd1f.1624911306.git.yann.morin.1998@free.fr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gitlab-ci: allow running test-pkg (branch yem/test-pkg-in-gitlab-ci) | expand |
On 28/06/2021 22:15, Yann E. MORIN wrote: > Currently, running test-pkg is only done locally on the developpers > machine. > > In a follow up commit, we'll add the possibility to run test-pkg in a > gitlab-ci pipeline and, to speed up things, with one job per buildable > configuration. > > As such, we will need that test-pkg only ever prepares the > configuration, and that it does not build them. > > Add such a mode, with a new option, --prepare-only > > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> > Cc: Romain Naour <romain.naour@gmail.com> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > > --- > Note: naming is hard; naming options is harder; naming options with a > terse term is even harder; naming options with a terse term that is > still meaningful and explains what the option does, is even harder yet. But doing it inconsistently is easy (see below). :-) Anyway, there's an easy solution to that (which I believe we should apply here): don't define a terse option. I think terse options should only be defined for stuff that a human has to type. In scripts, terse options shouldn't be used, because it makes it harder for the programmer to understand what the command does. Since prepare-only is meant ot be used by script, I don't think a terse option is needed. > > --- > v5: split off from the next patch into this patch > --- > utils/test-pkg | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/utils/test-pkg b/utils/test-pkg > index 54c6c5e8fe..4a20cab57f 100755 > --- a/utils/test-pkg > +++ b/utils/test-pkg > @@ -12,13 +12,13 @@ do_clean() { > > main() { > local o O opts > - local cfg dir pkg random toolchains_csv toolchain all number mode > + local cfg dir pkg random toolchains_csv toolchain all number mode prepare_only > local ret nb nb_skip nb_fail nb_legal nb_tc build_dir keep > local -a toolchains > local pkg_br_name > > - o='hakc:d:n:p:r:t:' > - O='help,all,keep,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:> + o='hakgc:d:n:p:r:t:' ^ AFAICS this is a 'g'... > + O='help,all,keep,prepare-only,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:' > opts="$(getopt -n "${my_name}" -o "${o}" -l "${O}" -- "${@}")" > eval set -- "${opts}" > > @@ -27,6 +27,7 @@ main() { > keep=0 > number=0 > mode=0 > + prepare_only=0 > toolchains_csv="${TOOLCHAINS_CSV}" > while [ ${#} -gt 0 ]; do > case "${1}" in > @@ -39,6 +40,9 @@ main() { > (-k|--keep) > keep=1; shift 1 > ;; > + (-l|--prepare-only) ^ ... but this is an 'l'! Clearly someone didn't test with the terse option. :-) > + prepare_only=1; shift 1 > + ;; > (-c|--config-snippet) > cfg="${2}"; shift 2 > ;; > @@ -127,7 +131,7 @@ main() { > toolchain="$(basename "${toolchainconfig}" .config)" > build_dir="${dir}/${toolchain}" > printf "%40s [%*d/%d]: " "${toolchain}" ${#nb_tc} ${nb} ${nb_tc} > - build_one "${build_dir}" "${toolchainconfig}" "${cfg}" "${pkg}" && ret=0 || ret=${?} > + build_one "${build_dir}" "${toolchainconfig}" "${cfg}" "${pkg}" "${prepare_only}" && ret=0 || ret=${?} > case ${ret} in > (0) printf "OK\n";; > (1) : $((nb_skip++)); printf "SKIPPED\n";; > @@ -147,6 +151,7 @@ build_one() { > local toolchainconfig="${2}" > local cfg="${3}" > local pkg="${4}" > + local defer="${5}" I like it if variables that are the same are called the same. I.e., use prepare_only here as well. Perhaps you can even skip passing it at all since it's a global variable. > > mkdir -p "${dir}" > > @@ -170,6 +175,11 @@ build_one() { > # Remove file, it's empty anyway. > rm -f "${dir}/missing.config" > > + # Defer building the job to the caller (e.g. a gitlab pipeline) > + if [ ${defer} -eq 1 ]; then > + return 0 > + fi > + > if [ -n "${pkg}" ]; then > if ! make O="${dir}" "${pkg}-dirclean" >> "${dir}/logfile" 2>&1; then > return 2 > @@ -257,6 +267,10 @@ Options: > Note: the logfile and configuration is always retained, even without > this option. > > + --prepare-only And the terse option isn't even in the help! So nobody would use it anyway... Regards, Arnout > + Only prepare the .config files, but do not build them. Output the > + list of build directories to stdout, and the status on stderr. > + > Example: > > Testing libcec would require a config snippet that contains: >
Hello Arnout, Yann, Le 05/08/2021 à 22:45, Arnout Vandecappelle a écrit : > > > On 28/06/2021 22:15, Yann E. MORIN wrote: >> Currently, running test-pkg is only done locally on the developpers >> machine. >> >> In a follow up commit, we'll add the possibility to run test-pkg in a >> gitlab-ci pipeline and, to speed up things, with one job per buildable >> configuration. >> >> As such, we will need that test-pkg only ever prepares the >> configuration, and that it does not build them. >> >> Add such a mode, with a new option, --prepare-only >> >> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> >> Cc: Romain Naour <romain.naour@gmail.com> >> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> >> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> >> >> --- >> Note: naming is hard; naming options is harder; naming options with a >> terse term is even harder; naming options with a terse term that is >> still meaningful and explains what the option does, is even harder yet. > > But doing it inconsistently is easy (see below). :-) > > Anyway, there's an easy solution to that (which I believe we should apply > here): don't define a terse option. > > I think terse options should only be defined for stuff that a human has to > type. In scripts, terse options shouldn't be used, because it makes it harder > for the programmer to understand what the command does. > > Since prepare-only is meant ot be used by script, I don't think a terse option > is needed. Thanks for your advice. Indeed I don't think we need a terse option. Best regards, Romain > > >> >> --- >> v5: split off from the next patch into this patch >> --- >> utils/test-pkg | 22 ++++++++++++++++++---- >> 1 file changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/utils/test-pkg b/utils/test-pkg >> index 54c6c5e8fe..4a20cab57f 100755 >> --- a/utils/test-pkg >> +++ b/utils/test-pkg >> @@ -12,13 +12,13 @@ do_clean() { >> >> main() { >> local o O opts >> - local cfg dir pkg random toolchains_csv toolchain all number mode >> + local cfg dir pkg random toolchains_csv toolchain all number mode prepare_only >> local ret nb nb_skip nb_fail nb_legal nb_tc build_dir keep >> local -a toolchains >> local pkg_br_name >> >> - o='hakc:d:n:p:r:t:' >> - O='help,all,keep,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:> + o='hakgc:d:n:p:r:t:' > ^ AFAICS this is a 'g'... > >> + O='help,all,keep,prepare-only,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:' >> opts="$(getopt -n "${my_name}" -o "${o}" -l "${O}" -- "${@}")" >> eval set -- "${opts}" >> >> @@ -27,6 +27,7 @@ main() { >> keep=0 >> number=0 >> mode=0 >> + prepare_only=0 >> toolchains_csv="${TOOLCHAINS_CSV}" >> while [ ${#} -gt 0 ]; do >> case "${1}" in >> @@ -39,6 +40,9 @@ main() { >> (-k|--keep) >> keep=1; shift 1 >> ;; >> + (-l|--prepare-only) > ^ ... but this is an 'l'! > > Clearly someone didn't test with the terse option. :-) > >> + prepare_only=1; shift 1 >> + ;; >> (-c|--config-snippet) >> cfg="${2}"; shift 2 >> ;; >> @@ -127,7 +131,7 @@ main() { >> toolchain="$(basename "${toolchainconfig}" .config)" >> build_dir="${dir}/${toolchain}" >> printf "%40s [%*d/%d]: " "${toolchain}" ${#nb_tc} ${nb} ${nb_tc} >> - build_one "${build_dir}" "${toolchainconfig}" "${cfg}" "${pkg}" && ret=0 || ret=${?} >> + build_one "${build_dir}" "${toolchainconfig}" "${cfg}" "${pkg}" "${prepare_only}" && ret=0 || ret=${?} >> case ${ret} in >> (0) printf "OK\n";; >> (1) : $((nb_skip++)); printf "SKIPPED\n";; >> @@ -147,6 +151,7 @@ build_one() { >> local toolchainconfig="${2}" >> local cfg="${3}" >> local pkg="${4}" >> + local defer="${5}" > > I like it if variables that are the same are called the same. I.e., use > prepare_only here as well. Perhaps you can even skip passing it at all since > it's a global variable. > >> >> mkdir -p "${dir}" >> >> @@ -170,6 +175,11 @@ build_one() { >> # Remove file, it's empty anyway. >> rm -f "${dir}/missing.config" >> >> + # Defer building the job to the caller (e.g. a gitlab pipeline) >> + if [ ${defer} -eq 1 ]; then >> + return 0 >> + fi >> + >> if [ -n "${pkg}" ]; then >> if ! make O="${dir}" "${pkg}-dirclean" >> "${dir}/logfile" 2>&1; then >> return 2 >> @@ -257,6 +267,10 @@ Options: >> Note: the logfile and configuration is always retained, even without >> this option. >> >> + --prepare-only > > And the terse option isn't even in the help! So nobody would use it anyway... > > > Regards, > Arnout > >> + Only prepare the .config files, but do not build them. Output the >> + list of build directories to stdout, and the status on stderr. >> + >> Example: >> >> Testing libcec would require a config snippet that contains: >>
Romain, Arnout, All, On 2021-08-21 15:38 +0200, Romain Naour spake thusly: > Le 05/08/2021 à 22:45, Arnout Vandecappelle a écrit : > > On 28/06/2021 22:15, Yann E. MORIN wrote: > >> Currently, running test-pkg is only done locally on the developpers > >> machine. > >> > >> In a follow up commit, we'll add the possibility to run test-pkg in a > >> gitlab-ci pipeline and, to speed up things, with one job per buildable > >> configuration. > >> > >> As such, we will need that test-pkg only ever prepares the > >> configuration, and that it does not build them. > >> > >> Add such a mode, with a new option, --prepare-only > >> > >> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> > >> Cc: Romain Naour <romain.naour@gmail.com> > >> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > >> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > >> > >> --- > >> Note: naming is hard; naming options is harder; naming options with a > >> terse term is even harder; naming options with a terse term that is > >> still meaningful and explains what the option does, is even harder yet. > > > > But doing it inconsistently is easy (see below). :-) > > > > Anyway, there's an easy solution to that (which I believe we should apply > > here): don't define a terse option. Ah, but there was a misunderstanding: I was refering to the "long option" that I tried to keep terse. I.e. I started off with: --just-generate-config-for-later-use-in-gitlab-CI-or-anyother-such-CI and eventually tried to shorten it as much as possible, while still keeping the meaning, so I ended up with just: --prepare-only As for the short, one-char option, indeed., we don't really need one. > > I think terse options should only be defined for stuff that a human has to > > type. In scripts, terse options shouldn't be used, because it makes it harder > > for the programmer to understand what the command does. > > > > Since prepare-only is meant ot be used by script, I don't think a terse option > > is needed. > Thanks for your advice. Indeed I don't think we need a terse option. We need a terse "long option", but we don;t need a one-char "short option". ;-) Yes, this is confusing... ;-) > >> + (-l|--prepare-only) > > ^ ... but this is an 'l'! > > Clearly someone didn't test with the terse option. :-) Yes, I did test with terse "long option", but not with the one-char "short option". Indeed. ;-] Regards, Yann E. MORIN.
diff --git a/utils/test-pkg b/utils/test-pkg index 54c6c5e8fe..4a20cab57f 100755 --- a/utils/test-pkg +++ b/utils/test-pkg @@ -12,13 +12,13 @@ do_clean() { main() { local o O opts - local cfg dir pkg random toolchains_csv toolchain all number mode + local cfg dir pkg random toolchains_csv toolchain all number mode prepare_only local ret nb nb_skip nb_fail nb_legal nb_tc build_dir keep local -a toolchains local pkg_br_name - o='hakc:d:n:p:r:t:' - O='help,all,keep,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:' + o='hakgc:d:n:p:r:t:' + O='help,all,keep,prepare-only,config-snippet:,build-dir:,number:,package:,random:,toolchains-csv:' opts="$(getopt -n "${my_name}" -o "${o}" -l "${O}" -- "${@}")" eval set -- "${opts}" @@ -27,6 +27,7 @@ main() { keep=0 number=0 mode=0 + prepare_only=0 toolchains_csv="${TOOLCHAINS_CSV}" while [ ${#} -gt 0 ]; do case "${1}" in @@ -39,6 +40,9 @@ main() { (-k|--keep) keep=1; shift 1 ;; + (-l|--prepare-only) + prepare_only=1; shift 1 + ;; (-c|--config-snippet) cfg="${2}"; shift 2 ;; @@ -127,7 +131,7 @@ main() { toolchain="$(basename "${toolchainconfig}" .config)" build_dir="${dir}/${toolchain}" printf "%40s [%*d/%d]: " "${toolchain}" ${#nb_tc} ${nb} ${nb_tc} - build_one "${build_dir}" "${toolchainconfig}" "${cfg}" "${pkg}" && ret=0 || ret=${?} + build_one "${build_dir}" "${toolchainconfig}" "${cfg}" "${pkg}" "${prepare_only}" && ret=0 || ret=${?} case ${ret} in (0) printf "OK\n";; (1) : $((nb_skip++)); printf "SKIPPED\n";; @@ -147,6 +151,7 @@ build_one() { local toolchainconfig="${2}" local cfg="${3}" local pkg="${4}" + local defer="${5}" mkdir -p "${dir}" @@ -170,6 +175,11 @@ build_one() { # Remove file, it's empty anyway. rm -f "${dir}/missing.config" + # Defer building the job to the caller (e.g. a gitlab pipeline) + if [ ${defer} -eq 1 ]; then + return 0 + fi + if [ -n "${pkg}" ]; then if ! make O="${dir}" "${pkg}-dirclean" >> "${dir}/logfile" 2>&1; then return 2 @@ -257,6 +267,10 @@ Options: Note: the logfile and configuration is always retained, even without this option. + --prepare-only + Only prepare the .config files, but do not build them. Output the + list of build directories to stdout, and the status on stderr. + Example: Testing libcec would require a config snippet that contains:
Currently, running test-pkg is only done locally on the developpers machine. In a follow up commit, we'll add the possibility to run test-pkg in a gitlab-ci pipeline and, to speed up things, with one job per buildable configuration. As such, we will need that test-pkg only ever prepares the configuration, and that it does not build them. Add such a mode, with a new option, --prepare-only Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> Cc: Romain Naour <romain.naour@gmail.com> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> --- Note: naming is hard; naming options is harder; naming options with a terse term is even harder; naming options with a terse term that is still meaningful and explains what the option does, is even harder yet. --- v5: split off from the next patch into this patch --- utils/test-pkg | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)