Unify docker devcontainer with dockerfile used for CI

[BloodStainedCrow]

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.

[BloodStainedCrow]

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

[FintasticMan]

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]

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

[BloodStainedCrow]

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

[FintasticMan]

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]

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[bot]]

Build size and comparison to main:	Section	Size	Difference
text	377480B	0B
data	940B	0B
bss	63540B	0B

[FintasticMan]

I don't think there should be any difference in build size... The build size it's being compared to is 512ebf07043510930256e3c12498046b510ef8a1, rather than 8e2dcda14fac3e9ebc3b592439ad1f2afbbc7076. For some reason it is comparing against the latest develop rather than the actual base commit.

This doesn't explain the build size difference though, as there are no code changes in develop since the base commit, nor any code changes in this PR. @Riksu9000, have you got any idea what's going on here? It seems that the build in the get-base-ref-size workflow is 16B larger than the build-firmware workflow for the same code.

[Riksu9000]

The workflow is intended to merge the latest develop and compare to the latest develop. I think the reason for the different commits relates to https://github.com/actions/checkout/issues/1036. It probably picked the commit when commits were pushed, not when running the workflow was approved, but in get-base-ref-size it always picks the latest one. This should be fixed by manually merging with the latest base_ref and a check to make sure the different jobs got the same commit.

As for the build size difference, I can only think of there being some variability between builds. 27c241c7ee51aa3b6e46c6472f7d975d9c702795 and 6dc49e5bdb86ffd5730eb10eb9c477276e79923c are the same size, as is the build from this PR. Only the get-base-ref-size differs.

To avoid straying off topic, let's ignore the build size in this PR.

[BloodStainedCrow]

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

[Riksu9000]

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

[BloodStainedCrow]

This pull request should be ready to merge now

[BloodStainedCrow]

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

[Riksu9000]

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

[BloodStainedCrow]

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.

[BloodStainedCrow]

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]

@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.

[BloodStainedCrow]

Sorry for the long radio silence. Life happened D: Also I screwed up the merge AGAIN. It shouldn't be so hard, should it...

[BloodStainedCrow]

If someone is willing to explain the dark art of updating a fork, without having all changes from the source branch showing up in my PR, to an apparent newbie, I would be forever grateful...

[mark9064]

The best way is probably to rebase:

• Get InfiniTimeOrg/InfiniTime main up to date (pull that branch)
• Switch to your branch
• git rebase <name of branch tracking InfiniTimeOrg/InfiniTime main>
• Resolve conflicts as needed
• Review afterwards with git log and check the sequence of commits is as you expect

Just in case things go super wrong, I'd make a copy of the entire repository folder before as a backup

[BloodStainedCrow]

Okay, I did as you described. According to git I can now pull 22 commits and push 44 commits onto my unifyDockerDevcontainer branch. Does that sound right? The git log output suggests my changes will be applied after all changes on main, which seems correct. I'd rather not have a third failed merge in the same PR xD

[FintasticMan]

That's right, but you shouldn't pull those 22 commits, you should force push the current state of your local branch to GitHub.

[mark9064]

Sounds good to me. The commit history should be something like

Oldest

• Fix code formatting
• Application selection at build time
• Fix link in build with VS ...
• Clean outdated comment
• (branch merge may still be here, doesn't matter if it is or isn't really)

Newest

If so you're good to force push

[BloodStainedCrow]

Bloody finally! Thank you so much. I will now continue my work from July.

[BloodStainedCrow]

@NeroBurner I have fixed all the issues you pointed out, as far as I know. There are 3 things though:

1. I noticed, in launch.json is a old launch configuration that no longer works. It has version pinetime-app-1.3.0 hardcoded and has therefore been broken since 1.4. It also reference a bunch of other files that I believe to no longer exist.
2. The docs for using VSCode/Devcontainers are very wrong/outdated. They are without any changes from this PR, and will be even more so afterwards. I plan on updating the docs, question is: Is this part of this PR or should I make another one for it?
3. There a couple of instances where I could not avoid hardcoding variables into launch-configs/settings etc. (It might be possible, but I have not found a way). If the version of clang-format/gdb ever changes, the devcontainer WILL break. Ideally we could have a test that fails if the hardcoded envs I use differ from the ones used in build.sh which I used as the baseline for everything. Then whenever the version is changed there, it must also be changed in the files for the devcontainer.

[BloodStainedCrow]

Also, if someone who does NOT use a devcontainer could verify, that I did not break anything with my changes, that would be great.

[BloodStainedCrow]

I also still need to test debugging from a devcontainer

[BloodStainedCrow]

When debugging I get the error error while loading shared libraries: libncurses.so.5: cannot open shared object file: No such file or directory when starting gdb. Will investigate further

[BloodStainedCrow]

Okay, debugging seems to work now, after adding libncurses5 to the container. I do not seem to get any source mapping, though. Is that normal?

[agittins]

It would be great to see this merged. I notice that the issues raised by @NeroBurner's review look to be resolved, and I was able to rebase this PR against main on my system without any issues, and the container builds for me OK.

Are any maintainers able to merge this, or will @BloodStainedCrow need to rebase and push first? It would be great to get the devcontainer setup working again so we can iterate any improvements from there.

[BloodStainedCrow]

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

1. Building the devcontainer failed due to bad CRLF (docker wants LF) line endings in build.sh

2. Compiling failed because of bad CRLF (the container/python/src/displayapp/fonts/generate.py) wants LF) in src/displayapp/fonts/jetbrains_mono_bold_20.*

3. Connecting the OpenOCD debugger failed (silently rage) because of a missing dependency: libncurses5

4. 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?

5. is simply fixed by adding the dependency to the dockerfile.

[BloodStainedCrow]

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

[BloodStainedCrow]

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]

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:

https://github.com/InfiniTimeOrg/InfiniTime/pull/1401#issuecomment-1296383632

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]

Yikes, thanks windows indeed

[NeroBurner]

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

[BloodStainedCrow]

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?

[BloodStainedCrow]

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]

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 😁