In Pillar, add new Makefile targets for Docker dev builds and shell access. #3575
Conversation
The Makefile has been updated to include two new targets: `build-docker-dev` and `enter-docker-dev`. These targets are designed to facilitate the evaluation of the codebase's state during the build process. The `build-docker-dev` target constructs a Docker image up to the 'build' stage, enabling developers to assess the intermediate state of the code, particularly useful when applying patches that might only be present in temporary containers and are difficult to inspect otherwise. Following the build, the `enter-docker-dev` target allows for immediate shell access within this intermediate Docker container, streamlining the process for developers to interact with the code and build environment directly. Signed-off-by: Nikolay Martyanov <nikolay@zededa.com>
|
Here is an example, where it can be useful: #3568 (comment) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3575 +/- ##
==========================================
+ Coverage 19.46% 19.47% +0.01%
==========================================
Files 231 231
Lines 50211 50211
==========================================
+ Hits 9774 9780 +6
+ Misses 39722 39714 -8
- Partials 715 717 +2 ☔ View full report in Codecov by Sentry. |
|
I delegate the review to @christoph-zededa. He is the docker-pocker expert. |
| docker build $(DOCKER_ARGS) -t $(DOCKER_TAG) . --target build | ||
|
|
||
| enter-docker-dev: build-docker-dev | ||
| docker run -it --rm --entrypoint /bin/sh $(DOCKER_TAG) |
There was a problem hiding this comment.
I normally add:
-v $(pwd):$(pwd)
this way I don't have to rebuild the container if I changed a file
There was a problem hiding this comment.
And how do you use it then? As far as I understand it will mount the host dir /pkg/pillar to / in the container, right?
There was a problem hiding this comment.
it mounts it to the same path in the container, f.e. for me it is: /home/christoph/projects/eve/pkg/pillar; then I do:
cd /path/to/mapped/directory
go test ./...
There was a problem hiding this comment.
Hm... In this case, will the host files in ${EVE_ROOT}/pkg/pillar replaced by the ones in the container?
If that's the case, I'm concerned that IDEs detecting disk changes might go haywire trying to reindex the directory structure that has been modified. Do you have any suggestions on how we could make this process more convenient for the developers?
There was a problem hiding this comment.
nothing will be replaced, it is just a bind-mount
Do you have any suggestions on how we could make this process more convenient for the developers?
Use a proper IDE? ;-)
There was a problem hiding this comment.
I'm not a fan of the "works for me" approach. I would prefer a solution suitable for more people...
Even with bind-mount - will it look like "replacing" the directory with that one from the container? I'm always confused with the order and priorities of these mounts...
There was a problem hiding this comment.
It will replace it, but I guess the normal container image has nothing in /home/user/....
I'm not a fan of the "works for me" approach. I would prefer a solution suitable for more people...
Then we should perhaps start first with investigating which IDEs have that problem and where exactly they fail.
P.S.: just feel free to ignore this discussion - you already have the approval from me.
There was a problem hiding this comment.
Yeah, sure, I hope we can merge the current simple solution.
Let's just use this thread to discuss what could the better solution look like.
So you say you mount ${HOST_EVE_ROOT}/pkg/pillar into ${HOST_EVE_ROOT}/pkg/pillar inside the container, right? In this case, you have 2 copies of source code in the container: 1 - /pillar/ - preprocessed code with the patches applied, 2 - ${HOST_EVE_ROOT}. Right? But in this case, if you change a file on your host - it will be visible only in the second path. So, the patched files are still invisible from the host and the tools/IDEs running there. Right?
There was a problem hiding this comment.
I normally do it from the eve directory, not pkg/pillar, but the consequences are the same, that I have the code twice in the container.
Yes, it will be only visible in the second path.
Yes, the patched files are invisible from the host and therefore also invisible for the IDEs.
There was a problem hiding this comment.
Yeah, sure, I hope we can merge the current simple solution.
Fine from my side.
LGTM |
| @@ -76,6 +76,12 @@ build-docker-$(APPS): $(DISTDIR) | |||
| build-docker: | |||
| docker build $(DOCKER_ARGS) -t $(DOCKER_TAG) . | |||
|
|
|||
| build-docker-dev: | |||
| docker build $(DOCKER_ARGS) -t $(DOCKER_TAG) . --target build | |||
There was a problem hiding this comment.
@OhmSpectator , I liked these targets, but unfortunately, they don't consider architecture, as the whole Makefile. In a nutshell, I cannot run enter-docker-dev for ARM.... You don't need to add it in this PR, but it would be good to have architecture support in the future, maybe we could create an issue/ticket for that...
There was a problem hiding this comment.
Hmmm... Good point. And just a question of curiosity, how do you make ARCH-specific steps for building Pillar now? Is there anything build-time specific in general?
There was a problem hiding this comment.
It's handled inside the Dockerfile to setup corresponding GO variables and cross-compilers (when needed): https://github.com/lf-edge/eve/blob/master/pkg/pillar/Dockerfile#L86
There was a problem hiding this comment.
So, the problem is that TARGETARCH is not set from Makefile, yeah?
There was a problem hiding this comment.
yes for go build ... commands, but in case of docker commands, it should be platform, like: --platform linux/arm64
There was a problem hiding this comment.
I think this is the only remaining comment before this can be merged.
There was a problem hiding this comment.
Yeah. I tried to devise a quick fix, but it did not work as expected. I'll come up with another solution and push it here. Meanwhile, switching the PR into Draft.
There was a problem hiding this comment.
@eriknordmark, as the new targets are not the only ones that lack multi-arch support, I would prefer to address this issue in the scope of a dedicated task, not blocking the PR. I'll create a ticket for that shortly.
OhmSpectator
changed the title
OhmSpectator
changed the title
There was a problem hiding this comment.
LGTM but let's have @rene agree we can add arm64 support to the targets later.
The Makefile has been updated to include two new targets:
build-docker-devandenter-docker-dev. These targets are designed to facilitate the evaluation of the codebase's state during the build process.The
build-docker-devtarget constructs a Docker image up to the 'build' stage, enabling developers to assess the intermediate state of the code, particularly useful when applying patches that might only be present in temporary containers and are difficult to inspect otherwise.Following the build, the
enter-docker-devtarget allows for immediate shell access within this intermediate Docker container, streamlining the process for developers to interact with the code and build environment directly.