| Message ID | 20220724054912.2354219-8-ricardo.martincoski@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Preventing style regressions using check-package | expand |
Hello Ricardo, Le 24/07/2022 à 07:49, Ricardo Martincoski a écrit : > Currently the result for both check-package and check-flake8 targets > depend on the version of the tools flake8 and shellcheck installed on > the host. > > In order to ensure reproducibility across machines of different > developers, always use the docker image to run these targets. > > When one of these targets is called, test if it is already running > inside the docker image, and if it not, start the docker image and run > the same command inside it. > Do the check for the docker image in a simple way: just check current > user belongs only to group br-user. It is very unlikely a developer has > exactly this user configuration in the host. > > Add a note in the Dockerfile about this use. > > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> > --- > Makefile | 10 ++++++++++ > support/docker/Dockerfile | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/Makefile b/Makefile > index bd7ab9675d..f42dc3151d 100644 > --- a/Makefile > +++ b/Makefile > @@ -1237,6 +1237,15 @@ release: > print-version: > @echo $(BR2_VERSION_FULL) > > +check_inside_docker := $(shell if [ "`groups`" = 'br-user' ]; then echo y; else echo n; fi) > + > +# List of target that need to run inside docker image to ensure reproducible results > +inside_docker_targets := check-package check-flake8 > + > +ifeq ($(check_inside_docker),n) > +$(inside_docker_targets): > + $(Q)utils/docker-run $(MAKE) V=$(V) $@ While I understand the reproducibility issue, I'm not sure I really wand my make check-package command starting starting a docker container. I guess Buildroot user (or CI build machine) can do the same if they want. Best regards, Romain > +else > check-flake8: > $(Q)git ls-tree -r --name-only HEAD \ > | xargs file \ > @@ -1246,6 +1255,7 @@ check-flake8: > > check-package: > $(Q)./utils/check-package `git ls-tree -r --name-only HEAD` > +endif > > include docs/manual/manual.mk > -include $(foreach dir,$(BR2_EXTERNAL_DIRS),$(sort $(wildcard $(dir)/docs/*/*.mk))) > diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile > index f54c31b54a..afe8911e78 100644 > --- a/support/docker/Dockerfile > +++ b/support/docker/Dockerfile > @@ -60,6 +60,7 @@ RUN sed -i 's/# \(en_US.UTF-8\)/\1/' /etc/locale.gen && \ > RUN useradd -ms /bin/bash br-user && \ > chown -R br-user:br-user /home/br-user > > +# Note: below user is used to check if we are running inside docker > USER br-user > WORKDIR /home/br-user > ENV HOME /home/br-user
Hello, On Wed, Jul 27, 2022 at 10:16 AM, Romain Naour wrote: [snip] >> +check_inside_docker := $(shell if [ "`groups`" = 'br-user' ]; then echo y; else echo n; fi) >> + >> +# List of target that need to run inside docker image to ensure reproducible results >> +inside_docker_targets := check-package check-flake8 >> + >> +ifeq ($(check_inside_docker),n) >> +$(inside_docker_targets): >> + $(Q)utils/docker-run $(MAKE) V=$(V) $@ > > While I understand the reproducibility issue, I'm not sure I really wand my make > check-package command starting starting a docker container. I really prefer a 'good default' with reproducible results. Advanced users can always call check-package directly. But perhaps some developer can hit some corporate policies when developing patches inside the company: - can't run docker at all - can't download docker images from the internet For that poor souls, I will withdraw this patch and resend. :) Well... we could instead change de manual to say: 299 --------------------- 300 $ utils/docker-run make check-package 301 --------------------- > > I guess Buildroot user (or CI build machine) can do the same if they want. CI builds already run inside the same docker image, so no change there. I was trying to accomplish with this series, among other things, a way to ensure the same results locally when compared to CI builds. One interesting reproducibility issue that is already taken into account by this series, even outside the docker image: patch 14 file utils/check-package line 98 I developed outside the docker image and when I run on CI build the python files were being not tested and ignored by check-package. See the reason: $ utils/docker-run python3 -c \ "import magic;" \ "print(magic.from_file('utils/brmake', mime=True));" \ "print(magic.from_file('utils/check-package', mime=True))"; text/x-shellscript text/x-script.python $ python3 -c \ "import magic;" \ "print(magic.from_file('utils/brmake', mime=True));" \ "print(magic.from_file('utils/check-package', mime=True))"; text/x-shellscript text/x-python But as I say above, I will withdraw this patch and resend the series. Regards, Ricardo
diff --git a/Makefile b/Makefile index bd7ab9675d..f42dc3151d 100644 --- a/Makefile +++ b/Makefile @@ -1237,6 +1237,15 @@ release: print-version: @echo $(BR2_VERSION_FULL) +check_inside_docker := $(shell if [ "`groups`" = 'br-user' ]; then echo y; else echo n; fi) + +# List of target that need to run inside docker image to ensure reproducible results +inside_docker_targets := check-package check-flake8 + +ifeq ($(check_inside_docker),n) +$(inside_docker_targets): + $(Q)utils/docker-run $(MAKE) V=$(V) $@ +else check-flake8: $(Q)git ls-tree -r --name-only HEAD \ | xargs file \ @@ -1246,6 +1255,7 @@ check-flake8: check-package: $(Q)./utils/check-package `git ls-tree -r --name-only HEAD` +endif include docs/manual/manual.mk -include $(foreach dir,$(BR2_EXTERNAL_DIRS),$(sort $(wildcard $(dir)/docs/*/*.mk))) diff --git a/support/docker/Dockerfile b/support/docker/Dockerfile index f54c31b54a..afe8911e78 100644 --- a/support/docker/Dockerfile +++ b/support/docker/Dockerfile @@ -60,6 +60,7 @@ RUN sed -i 's/# \(en_US.UTF-8\)/\1/' /etc/locale.gen && \ RUN useradd -ms /bin/bash br-user && \ chown -R br-user:br-user /home/br-user +# Note: below user is used to check if we are running inside docker USER br-user WORKDIR /home/br-user ENV HOME /home/br-user
Currently the result for both check-package and check-flake8 targets depend on the version of the tools flake8 and shellcheck installed on the host. In order to ensure reproducibility across machines of different developers, always use the docker image to run these targets. When one of these targets is called, test if it is already running inside the docker image, and if it not, start the docker image and run the same command inside it. Do the check for the docker image in a simple way: just check current user belongs only to group br-user. It is very unlikely a developer has exactly this user configuration in the host. Add a note in the Dockerfile about this use. Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> --- Makefile | 10 ++++++++++ support/docker/Dockerfile | 1 + 2 files changed, 11 insertions(+)