Unify docker devcontainer with dockerfile used for CI #1587

BloodStainedCrow · 2023-02-14T11:46:18Z

As discussed in #1497 this removes the need for duplicate Dockerfiles for Docker compilation and .devcontainer development. This will reduce variance in compiler environments and make maintenance easier.

FintasticMan

I think it would be nice to remove the .devcontainer directory, and move the JSON to .devcontainer.json in the root. We would need to move the documentation somewhere else though...

docker/Dockerfile

.devcontainer/devcontainer.json

BloodStainedCrow · 2023-02-14T12:46:02Z

I would put the .devcontainer/README.md into docs/usingDevcontainers.md, maybe?

FintasticMan · 2023-02-14T13:59:02Z

I would put the .devcontainer/README.md into docs/usingDevcontainers.md, maybe?

Seems good to me. One more thing, make sure to add a trailing line feed to files, so that the diff doesn't complain.

BloodStainedCrow · 2023-02-14T14:00:03Z

Oh, I thought that would be automatically done via the linter. Will do

BloodStainedCrow · 2023-02-14T14:01:03Z

Nevermind. The linter automatically removes trailing newlines... Is that intended?

FintasticMan · 2023-02-14T14:03:46Z

Which linter are you using? clang-format should add trailing newlines, or at the very least not remove them. We mostly stick to having trailing newlines, and I think that almost all files already have them. (Keep in mind that clang-format is only for C/C++ files)

BloodStainedCrow · 2023-02-14T14:04:49Z

Well, I am using the standard linter integrated into the devcontainer. The linter automatically removes trailing endlines at the end of .json files.

github-actions · 2023-02-16T07:08:19Z

Build size and comparison to main:

Section	Size	Difference
text	377480B	0B
data	940B	0B
bss	63540B	0B

Riksu9000

The currently active maintainers don't use the devcontainer, so you'll need to explain what these changes do.

Please read the commit conventions https://github.com/InfiniTimeOrg/InfiniTime/blob/develop/doc/contribute.md#commit-conventions

.vscode/cmake-kits.json

docker/Dockerfile

.vscode/c_cpp_properties.json

BloodStainedCrow · 2023-02-17T09:28:55Z

Question: what exactly is the correct compiler we are using? I need to specify the compiler "executable" for Intellisense

Riksu9000 · 2023-02-19T11:49:04Z

It's the one specified in the build instructions. https://github.com/InfiniTimeOrg/InfiniTime/blob/develop/doc/buildAndProgram.md

BloodStainedCrow · 2023-02-22T16:14:43Z

This pull request should be ready to merge now

FintasticMan

Sorry it's taken so long to get around to reviewing this. I think that most of it is pretty good. I think it should be possible to get the executable paths etc. from build.sh by exporting them to the environment.

.devcontainer.json

docker/Dockerfile

.devcontainer.json

BloodStainedCrow · 2023-03-16T11:45:23Z

Why are the checks failing? The logs imply a check called Compare build size is missing? I doubt that is intended behaviour?

Riksu9000 · 2023-03-16T12:26:35Z

This check depends on another check, but they can't be run because there are conflicts.

BloodStainedCrow · 2023-03-20T09:09:05Z

I have looked into extracting the compilerPath from build.sh. I do not see a way to do that currently. It was possible with the cmake extension for vscode since it allows to set a .sh script for setting up env variables. But the Intellisense (C/Cpp) Extension does not seem to support that sadly.
As far as I know this means either there will continue to be a possible desync of the two files or defines like the compiler, clang-format versions or paths need to be centrally defined in a .env file. In either case, that would be out of scope for this PR I think.

If anyone knows if there is way to set VSCode extension settings based on exported variables in a .sh script that would be ideal, I did not find a way.

FintasticMan

While it's not perfect, mostly due to the copying of variables, this fixes the devcontainer and improves its maintainability.

BloodStainedCrow · 2023-04-20T09:21:45Z

Uh oh, I see the PR now has 176 changed files, did I mess up the merge...?
I merged all commits from main into my branch to get rid of the conflicts, why are all files that were changed on main now part of this PR?

FintasticMan · 2023-07-11T12:00:40Z

@BloodStainedCrow I've just fixed that merge commit, so now I believe this pull request is in a good enough state to be squash merged.

NeroBurner

Thanks for reducing redundancy. But the current version with hard coded paths breaks existing setups. Which isn't desirable

.vscode/launch.json

.vscode/settings.json

docker/Dockerfile

NeroBurner

small open questions from my side. Everything else looking sound. I don't use the Dockerfile or the devcontainer, so I didn't check functionality.

Would love for someone else to chime in and do a review as well

.vscode/c_cpp_properties.json

.vscode/cmake-kits.json

NeroBurner · 2024-03-21T13:15:35Z

.vscode/launch.json

+            // FIXME: This is hardcoded. I have no idea how to use the values set in build.sh here
+            "gdbPath": "/opt/gcc-arm-none-eabi-10.3-2021.10/bin/arm-none-eabi-gdb",


can the "GCC_ARM_PATH" variable be used here?

As far as I know (see my previous attempts above) VSCode currently does not support loading variables for launch.json from anything but launch.json itself, which sadly means without some serious hacks, there will be this reference here, which might become outdated.

Looking at this though, the very least I should do is unify the occurrences of it in a variable in launch.json so only one value can grow stale

that would be very welcome, as it would minimize the staleness factor (if that is a word 😁 )

I was mistaken, it seems is not possible to share variables (outside a .env file, but that would be out of scope for this PR IMO) between mutiple launch.json configurations.

Additionally I currently have no way to test the other configuration on, whether they even still work in their current state, from what I can tell, at least one of them is around 3 years out of date...

Not a different compiler? We just need one (the currently officially supported one)

Or what configurations are you talking about. If they are deprecated the maybe we can remove those?

In launch.json there are different launch configurations usable via a button in VSCode. These are what I meant.

While I am open to removing them/replacing them with ones that actually work, I would probably like some more input from people who might use them, so maybe in another PR...

Thanks for the clarification. I agree. Let's merge this!

BloodStainedCrow · 2024-03-21T14:40:07Z

I tried using the devcontainer again, and found a couple snags:

Building the devcontainer failed due to bad CRLF (docker wants LF) line endings in build.sh
Compiling failed because of bad CRLF (the container/python/src/displayapp/fonts/generate.py) wants LF) in src/displayapp/fonts/jetbrains_mono_bold_20.*
Connecting the OpenOCD debugger failed (silently rage) because of a missing dependency: libncurses5
and 2. were already known by me: I currently attribute them to me using WSL2 on Windows and Docker Desktop which is known to have some bugs leading to weirdness with line endings. If either someone else who knows a little about devcontainers (or me depends on who is faster) can test it on a Linux with docker machine and see if that changes anything.
I presume changing the line endings on these files is a breaking change for some people and therefore not desired?
is simply fixed by adding the dependency to the dockerfile.

BloodStainedCrow · 2024-03-21T14:40:38Z

Additionally there does not seem to be any source mapping for debugging, I will have to look into that aswell

BloodStainedCrow · 2024-03-21T15:07:12Z

Okay, I have now found the correct setting to add source/symbol maps to gdb for debugging. As far as I can tell: pinetime-app-1.14.0.map is the correct symbol map. But gdb errors out with: can't read symbols: file format not recognized.
I was unable to find any info on using .map files with gdb so either I do not know what the correct symbol map file is, or I am missing something else. Is pinetime-app-1.14.0.map correct?

agittins · 2024-03-21T16:15:03Z

4. I currently attribute them to me using WSL2 on Windows

Indeed. The line endings however is due to git using the local encoding when you check out the repo (ie, in windows, so it gives them CR/LFs) but then when that repo is accessed inside the container all the tools expect unix textfiles.

So the actual repo doesn't contain any CR/LF files, they get converted to that when you check them out under windows.

We hit this when I was first trying to tackle the whole devcontainers thing. I think this comment provided the best general solution:

#1401 (comment)

Even Microsoft itself recommends setting eol=lf when working with devcontainers on Windows.

Another option could be to instruct Windows users to clone with --config core.eol=lf if they want to use devcontainers. > That wouldn't be seamless, but I suppose there really isn't any way to make this completely seamless (thanks Windows).

Ie, if you do the initial checkout in windows with eol=lf then it all works, and not doing so doesn't have any effects on those not using devcontainers.

BloodStainedCrow · 2024-03-21T16:17:12Z

Yikes, thanks windows indeed

NeroBurner · 2024-03-21T17:55:56Z

@BloodStainedCrow could you recheck your open points with a LF checked out repo on WSL?

BloodStainedCrow · 2024-03-22T13:49:36Z

Both the failed container build and the failed compile are solved with this.

The only remaining problem I encountered is the missing source map/ symbol file for debugging.
Do you know which is the correct symbol file for gdb?

NeroBurner

Thanks for keep on going, improving and keeping this PR up to date!

BloodStainedCrow · 2024-03-23T07:39:56Z

No problem, one question before I agree this can be merged:

The only remaining problem I encountered is the missing source map/ symbol file for debugging. Do you know which is the correct symbol file for gdb?

NeroBurner · 2024-03-23T09:46:39Z

No problem, one question before I agree this can be merged:

The only remaining problem I encountered is the missing source map/ symbol file for debugging. Do you know which is the correct symbol file for gdb?

I don't sorry. Let's fix that in a separate PR 😁

…1587) * Only use one Dockerfile and build.sh script for both docker and devcontainer * Remove all now unneccessary tasks and scripts * Update to clang-format-14 * Move devcontainer.json into root folder * Fix conditional statements in Dockerfile * Move .devcontainer/README into doc/usingDevcontainers * Remove obsolete VSCode Task * Change standard compiler path to the correct compiler * Set GDB Path for debugging * Hide broken buttons from CMake Extension * Refactor .devcontainer * Remove unneccessary postBuildCommand * Add devcontainer dependencies to all docker images * Add Devcontainer Debug launch config * Add an additional c_cpp_properties config as a fallback for devcontainer * Remove obsolete Docker Argument * Fix wrong C/Cpp versions * Fix silent fail of gdb, add libncurses5

FintasticMan reviewed Feb 14, 2023

View reviewed changes

docker/Dockerfile Outdated Show resolved Hide resolved

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved

BloodStainedCrow marked this pull request as ready for review February 14, 2023 12:50

This comment was marked as off-topic.

Sign in to view

Riksu9000 reviewed Feb 17, 2023

View reviewed changes

.vscode/cmake-kits.json Outdated Show resolved Hide resolved

docker/Dockerfile Outdated Show resolved Hide resolved

docker/Dockerfile Outdated Show resolved Hide resolved

.vscode/c_cpp_properties.json Outdated Show resolved Hide resolved

Riksu9000 requested a review from FintasticMan February 23, 2023 07:09

agittins mentioned this pull request Mar 7, 2023

Update vscode configuration for devcontainers #1401

Open

9 tasks

FintasticMan requested changes Mar 15, 2023

View reviewed changes

.devcontainer.json Outdated Show resolved Hide resolved

.devcontainer.json Outdated Show resolved Hide resolved

docker/Dockerfile Outdated Show resolved Hide resolved

.devcontainer.json Outdated Show resolved Hide resolved

.devcontainer.json Outdated Show resolved Hide resolved

BloodStainedCrow requested a review from FintasticMan April 11, 2023 11:56

FintasticMan approved these changes Apr 11, 2023

View reviewed changes

FintasticMan added the maintenance Background work label Jun 22, 2023

FintasticMan force-pushed the unifyDockerDevcontainer branch from 7cbc2a8 to 647b77e Compare July 11, 2023 11:47

FintasticMan requested a review from a team July 11, 2023 12:00

NeroBurner requested changes Jul 11, 2023

View reviewed changes

.vscode/launch.json Outdated Show resolved Hide resolved

.vscode/launch.json Outdated Show resolved Hide resolved

.vscode/launch.json Outdated Show resolved Hide resolved

.vscode/settings.json Outdated Show resolved Hide resolved

docker/Dockerfile Outdated Show resolved Hide resolved

FintasticMan mentioned this pull request Sep 3, 2023

Nodesource script is deprecated #1846

Closed

1 task

BloodStainedCrow added 10 commits March 21, 2024 12:15

Hide broken buttons from CMake Extension

3a58fde

Refactor .devcontainer

e8477c7

Remove unneccessary runArgs

a565015

Remove unneccessary postBuildCommand

7d6bff9

Add devcontainer dependencies to all docker images

1dc1609

Clean outdated comment

2826a25

Add Devcontainer Debug launch config

9b2c1b6

Add an additional c_cpp_properties config as a fallback for devcontainer

de77ef8

Remove obsolete Docker Argument

077af79

Re-Add stopping on main after restart

0a0a4c3

BloodStainedCrow force-pushed the unifyDockerDevcontainer branch from 58715cc to 0a0a4c3 Compare March 21, 2024 11:17

BloodStainedCrow requested a review from NeroBurner March 21, 2024 11:19

NeroBurner requested changes Mar 21, 2024

View reviewed changes

NeroBurner requested a review from a team March 21, 2024 13:18

Fix wrong C/Cpp versions

f0e3984

Fix silent fail of gdb, add libncurses5

c326cd9

NeroBurner approved these changes Mar 23, 2024

View reviewed changes

NeroBurner merged commit 57e625d into InfiniTimeOrg:main Mar 23, 2024
6 checks passed

BloodStainedCrow mentioned this pull request Mar 23, 2024

Using GDB debugger in devcontainer does not load source map #2044

Open

1 task

FintasticMan added this to the 1.15.0 milestone Nov 21, 2024

		// FIXME: This is hardcoded. I have no idea how to use the values set in build.sh here
		"gdbPath": "/opt/gcc-arm-none-eabi-10.3-2021.10/bin/arm-none-eabi-gdb",

Unify docker devcontainer with dockerfile used for CI #1587

Unify docker devcontainer with dockerfile used for CI #1587

Conversation

BloodStainedCrow commented Feb 14, 2023

FintasticMan left a comment

Choose a reason for hiding this comment

BloodStainedCrow commented Feb 14, 2023

FintasticMan commented Feb 14, 2023

BloodStainedCrow commented Feb 14, 2023

BloodStainedCrow commented Feb 14, 2023

FintasticMan commented Feb 14, 2023 • edited Loading

BloodStainedCrow commented Feb 14, 2023 • edited Loading

github-actions bot commented Feb 16, 2023 • edited Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

Riksu9000 left a comment

Choose a reason for hiding this comment

BloodStainedCrow commented Feb 17, 2023

Riksu9000 commented Feb 19, 2023

BloodStainedCrow commented Feb 22, 2023

FintasticMan left a comment

Choose a reason for hiding this comment

BloodStainedCrow commented Mar 16, 2023

Riksu9000 commented Mar 16, 2023

BloodStainedCrow commented Mar 20, 2023

FintasticMan left a comment

Choose a reason for hiding this comment

BloodStainedCrow commented Apr 20, 2023 • edited Loading

FintasticMan commented Jul 11, 2023

NeroBurner left a comment

Choose a reason for hiding this comment

NeroBurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BloodStainedCrow Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BloodStainedCrow commented Mar 21, 2024

BloodStainedCrow commented Mar 21, 2024

BloodStainedCrow commented Mar 21, 2024

agittins commented Mar 21, 2024

BloodStainedCrow commented Mar 21, 2024

NeroBurner commented Mar 21, 2024

BloodStainedCrow commented Mar 22, 2024

NeroBurner left a comment

Choose a reason for hiding this comment

BloodStainedCrow commented Mar 23, 2024

NeroBurner commented Mar 23, 2024

FintasticMan commented Feb 14, 2023 •

edited

Loading

BloodStainedCrow commented Feb 14, 2023 •

edited

Loading

github-actions bot commented Feb 16, 2023 •

edited

Loading

BloodStainedCrow commented Apr 20, 2023 •

edited

Loading

BloodStainedCrow Mar 21, 2024 •

edited

Loading