Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile.include: pass IOTLAB_NODE to docker #17545

Merged

Conversation

fjmolinas
Copy link
Contributor

Contribution description

Release testing I cam upon this corner case, not sure this is the best fix, open to ideas.

By default if IoT-LAB Cli Tools V3 is used then BINFILE is used to
flash on IoT-LAB. But BINFILE is not built by default when RIOT_CI_BUILD
is set as a ci optimization.

But since before IOTLAB_NODE was not passed to docker when building it
did not know that it should BUILD BINFILE as well, which led to failures
if doing:

$ IOTLAB_NODE=iotlab-m3.grenoble.iot-lab.info BOARD=iotlab-m3
RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 make -C examples/hello-world/ flash

But if IOTLAB_NODE is passed at is checks for IoT-LAB cli Tools also
happen in the docker container which leads to a make error since those
are not present in docker.

Therefore avoid including the IoT-LAB makefiles when in docker, but add
BINFILE to BUILD_FILES if IOTLAB_NODE is set

Testing procedure

$ IOTLAB_NODE=iotlab-m3.103.grenoble.iot-lab.info BOARD=iotlab-m3
RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 make -C examples/hello-world/ flash

Fails in master succeeds with this PR

@fjmolinas fjmolinas requested a review from aabadie January 20, 2022 17:39
@github-actions github-actions bot added the Area: build system Area: Build system label Jan 20, 2022
@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 21, 2022
@fjmolinas
Copy link
Contributor Author

@aabadie

@aabadie
Copy link
Contributor

aabadie commented Jan 24, 2022

In the test-on-iotlab GH action, only adapting BUILD_FILES is needed:

# Force .bin files generation because these files are used to flash on IoT-LAB and
# because compile_and_test_for_board forces RIOT_CI_BUILD which skip .bin
# files generation
DOCKER_ENVIRONMENT_CMDLINE: -e BUILD_FILES=\$$\(BINFILE\)

I don't know how but IOTLAB_NODE is set correctly even when using BUILD_IN_DOCKER (or this action is also broken).

@aabadie
Copy link
Contributor

aabadie commented Jan 24, 2022

I don't know how but IOTLAB_NODE is set correctly even when using BUILD_IN_DOCKER

I know: IOTLAB_NODE doesn't need to be known from Docker, since it's only used by the flasher.

Makefile.include Outdated
@@ -385,6 +385,7 @@ export PREFIX ?= $(if $(TARGET_ARCH),$(TARGET_ARCH)-)
ifneq (,$(IOTLAB_NODE))
PROGRAMMER ?= iotlab
# iotlab uses ELFFILE by default for flashing boards.
BUILD_FILES += $(BINFILE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would only do this is RIOT_CI_BUILD is set:

Suggested change
BUILD_FILES += $(BINFILE)
ifeq (1,$(RIOT_CI_BUILD))
BUILD_FILES += $(BINFILE)
endif

@@ -67,6 +67,7 @@ export DOCKER_ENV_VARS += \
SIZE \
TOOLCHAIN \
TEST_KCONFIG \
IOTLAB_NODE \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep the alphabetical order (which as I saw it is already a bit broken...) ?

@aabadie
Copy link
Contributor

aabadie commented Jan 24, 2022

I came up with a similar approach (not sure it's better):

diff --git a/Makefile.include b/Makefile.include
index a91c125ecc..97810cece7 100644
--- a/Makefile.include
+++ b/Makefile.include
@@ -386,6 +386,17 @@ ifneq (,$(IOTLAB_NODE))
   PROGRAMMER ?= iotlab
   # iotlab uses ELFFILE by default for flashing boards.
   FLASHFILE ?= $(ELFFILE)
+  # RIOT_CI_BUILD disables the build of BINFILE which is required for flashing
+  # on IoT-LAB
+  ifeq (1,$(RIOT_CI_BUILD))
+    BUILD_FILES += $(BINFILE)
+  endif
+  # Disable IOTLAB_NODE if inside Docker to avoid including the
+  # iotlab.single.inc.mk file which is useless there: it's only useful for
+  # flashing and this is done outside of Docker.
+  ifeq (1,$(INSIDE_DOCKER))
+    IOTLAB_NODE :=
+  endif
 endif
 
 # Add standard include directories
diff --git a/makefiles/docker.inc.mk b/makefiles/docker.inc.mk
index cfa6cc7c4e..c32a244993 100644
--- a/makefiles/docker.inc.mk
+++ b/makefiles/docker.inc.mk
@@ -47,6 +47,7 @@ export DOCKER_ENV_VARS += \
   ELFFILE \
   HEXFILE \
   FLASHFILE \
+  IOTLAB_NODE \
   LINK \
   LINKFLAGPREFIX \
   LINKFLAGS \

The test-on-iotlab GH action has also to be updated.

By default if IoT-LAB Cli Tools V3 is used then BINFILE is used to
flash on IoT-LAB. But BINFILE is not built by default when RIOT_CI_BUILD
is set as a ci optimization.

But since before IOTLAB_NODE was not passed to docker when building it
did not know that it should BUILD BINFILE as well, which led to failures
if doing:

$ IOTLAB_NODE=iotlab-m3.grenoble.iot-lab.info BOARD=iotlab-m3 \
  RIOT_CI_BUILD=1 BUILD_IN_DOCKER=1 make -C examples/hello-world/ flash

But if IOTLAB_NODE is passed at is checks for IoT-LAB cli Tools also
happen in the docker container which leads to a make error since those
are not present in docker.

Therefore add BINFILE to BUILD_FILES if RIOT_CI_BUILD is set, but unset
IOTLAB_NODE once INSIDE_DOCKER.
@fjmolinas fjmolinas force-pushed the pr_iotlab_node_docker_riotci_build branch from cb9589f to d2b9b4c Compare January 27, 2022 13:58
@github-actions github-actions bot added the Area: CI Area: Continuous Integration of RIOT components label Jan 27, 2022
@fjmolinas
Copy link
Contributor Author

Applied suggested changes

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running the test-on-iotlab action on my fork and it works ok.

Code changes are good.

ACK

@aabadie aabadie enabled auto-merge January 27, 2022 14:12
@aabadie aabadie merged commit 6371051 into RIOT-OS:master Jan 27, 2022
@fjmolinas fjmolinas deleted the pr_iotlab_node_docker_riotci_build branch March 24, 2022 08:31
@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants