InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.75k stars 941 forks source link

Update vscode configuration for devcontainers #1401

Open agittins opened 2 years ago

agittins commented 2 years ago

The existing vscode config (in .vscode and .devcontainers) included duplication of the Dockerfile and build.sh from docker/. Over time these copies have fallen behind the current versions and present a maintenance overhead.

This PR reconfigures how the devcontainer set-up is done, so that it uses the main container built by docker/ (either by using a locally built image, or by pulling one from dockerhub) and wraps it with a thin image to support the required vscode-specific bits. This way we only have one build.sh and one builder docker image to maintain.

This is a first iteration of this cleanup. I expect that the other scripts inside .devcontainer can probably be removed or converted into options of build.sh and the configurations can be make more generic / work-out-of-the-box, but I need a bit more experience / more eyes on the issue in order to move forward on that, as I am pretty green on the whole c++/cmake/vscode thing - but I can docker. :-)

TODOs for this PR:

Feedback / review especially welcome from anyone who can test this on their system with vscode.

agittins commented 2 years ago

Quick heads-up for anyone testing!

image image image

Elara6331 commented 2 years ago

I'm testing on Windows, macOS, and Linux.

On Linux, it worked first try.

On Windows, it seems like it would work, but there are issues with line endings. Specifically, bash keeps complaining about /rs everywhere.

Here's what I mean:

Screenshot_2022-10-29_12-57-55

This is after I ran dos2unix on /opt/build.sh. Before that, the script didn't start at all.

My macOS system is still building the devcontainer, I'll get back to you with the results of that one.

agittins commented 2 years ago

I'm testing on Windows, macOS, and Linux.

Huzzah! Thanks for trying it out!

On Linux, it worked first try.

Yay!

On Windows, it seems like it would work, but there are issues with line endings. Specifically, bash keeps complaining about /rs everywhere.

...

This is after I ran dos2unix on /opt/build.sh. Before that, the script didn't start at all.

Eep! That's wild. Do you recall what the error was before you tried the dos2unix trick? I wonder how vscode handles that generally, you'd think it'd have to have a way. Do you have other linux-based devcontainers that work on the windows system? Just to rule out local config vs this devcontainer. There are some tips at https://code.visualstudio.com/docs/devcontainers/tips-and-tricks that might be worth experimenting with, although you might already be familiar with all that.

Oh wait... I think what's happening is that there's a bodgy step I'm doing in .devcontainer/Dockerfile, where I try to symlink the "latest" version of the toolchain and SDK folders to a predictable name. I do the same thing in build.sh but in a much less dodgy way, but there's a chicken/egg issue at least until the next builder image gets published.

If it lets you into the container, can you try creating the two symlinks manually?

$ cd /opt
$ ln -s gcc-arm-<tabtab!> gcc-arm-none-eabi
$ ln -s nRF5_SDK_<tab!> nRF5_SDK

That should create the links (beware, the links might already exist, and might point at newline-named-dirs so you might need to rm them first). If you get that solved, see if you can run /opt/build.sh again (with or without the dos2unix step).

If the symlinks do already exist (with dumb target names) then you could try just rming them, then run /opt/build.sh again - since build.sh should create the symlinks if they're not already there, and it has the right knowledge to not create them in a stupidly wrong way! :-)

If that solves it I can just hard-code the version in the devcontainer dockerfile, since that particular problem should go away once we have a new published builder image.

Thanks again for taking the time on this!

My macOS system is still building the devcontainer, I'll get back to you with the results of that one.

Fingers crossed!

Elara6331 commented 2 years ago

Eep! That's wild. Do you recall what the error was before you tried the dos2unix trick? I wonder how vscode handles that generally, you'd think it'd have to have a way.

Yeah, there were just a bunch of bash syntax errors, and it also tried to use \r as a command a few times and couldn't find one named that (obviously).

If that solves it I can just hard-code the version in the devcontainer dockerfile, since that particular problem should go away once we have a new published builder image.

You're probably right. Your latest commit fixed the script itself (no bash errors, even with un-dos2unix-ed build.sh), but when compiling, there is an error:

Screenshot_2022-10-29_14-40-53

I don't know if this is an issue with the actual devcontainer though, it looks like there's a problem with a patch file.

Edit: Actually, the _M in the filename of the patch might be ^M with the ^ changed to _. If so, this is another line ending issue, since ^M is a carriage return.

Just saw that it's named with _M in the repo.

Fingers crossed!

It's compiling InfiniTime right now. Might take a while on its 2nd gen i5.

Elara6331 commented 2 years ago

MacOS finally finished building. No errors, everything worked great. It's just Windows and their \r\n nonsense causing failures.

Elara6331 commented 2 years ago

After running dos2unix on jetbrains_mono_bold_20.c_M.patch, that error is gone. I think this is caused by git trying to normalize line endings when cloning the repo. Editing the .gitattributes file in the root of the repo to set eol=lf on some files might fix it.

Edit: Yep, looks like core.autocrlf is set to true on Windows by default, but unset on Linux. I have no idea why it has different default values on different OSes, but it seems to be messing with the commands, since they're running in a Linux container and expect LF rather than CRLF. In this case, we can also instruct users to clone using git clone --config core.autocrlf=false https://github.com/InfiniTimeOrg/InfiniTime if they're on Windows and want to use devcontainers, because forcing the use of LF will cause native (non-containerized) Windows builds to break (if that's a concern, I'm not sure if they work currently).

agittins commented 2 years ago

After running dos2unix on jetbrains_mono_bold_20.c_M.patch, that error is gone. I think this is caused by git trying to normalize line endings when cloning the repo. Editing the .gitattributes file in the root of the repo to set eol=lf on some files might fix it.

Edit: Yep, looks like core.autocrlf is set to true on Windows by default, but unset on Linux. I have no idea why it has different default values on different OSes, but it seems to be messing with the commands, since they're running in a Linux container and expect LF rather than CRLF. In this case, we can also instruct users to clone using git clone --config core.autocrlf=false https://github.com/InfiniTimeOrg/InfiniTime if they're on Windows and want to use devcontainers, because forcing the use of LF will cause native (non-containerized) Windows builds to break (if that's a concern, I'm not sure if they work currently).

Great work chasing that down! Hmmm... at https://code.visualstudio.com/docs/devcontainers/tips-and-tricks it suggests adding to .gitattributes:

* text=auto eol=lf
*.{cmd,[cC][mM][dD]} text eol=crlf
*.{bat,[bB][aA][tT]} text eol=crlf

However they're mentioning it mainly for avoiding the case where git continually raises file modifications due to line endings, I'm not clear on how it's meant to work with respect to cloning a repo on windows then mounting it into a linux container.

Personally, what I'd love is to have a workflow where anyone can use the generic instructions to clone the repo, fire up vscode and be able to build the full release without having to do anything out of the ordinary.

If that patch was the only file you had to change (given there are other font patches there) then maybe it's just something about the way that particular patch is done. I'll have a dig. Maybe the whole git/devcontainer flow is doing the right thing with line-endings, but we're tripping a few corner cases.

Thanks again for the help!

Elara6331 commented 2 years ago

No, I've just encountered another error involving windows CRLF. Specifically, something attempted to execute python\r. It does look like the entire repo might need to be forced to LF, which I actually think that snippet should do just fine, since it will force git to always normalize to LF unless the file extension is .bat or .cmd.

Elara6331 commented 2 years ago

The root issue here is that Windows expects CRLF while Linux expects LF. Git normalizes to CRLF because it assumes you'll be building with Windows tools, but since it's in a Linux container, it has to use LF, because nearly all Linux tools expect LF. I imagine the reason that's the only patch I had to convert is because that particular patch contains a newline that was normalized by git. However, these issues will keep happening every time someone adds anything new unless the repo is LF.

agittins commented 2 years ago

Hmm. I'd totally be all for just enforcing non-daft-line-endings, but it just depends on whether building on bare windows is considered to be a "supported" or desired build environment. Given the ubiquitous availability of dockerized environments and the easier support load they result in I'd personally go for "no", but I don't know if that aligns with the project's goals.

I note that we already have a .gitattributes file in the repo, and it sets * text=auto and then specifies some extensions.

If building in docker is decreed as the only truly "supported" environment (which I'd vote for!) then we could just force LF across the board. @JF002 I think we've hit a "policy" ifdef, if you have a moment to weigh in that would be awesome.

Elara6331 commented 2 years ago

I am currently trying locally with a repo cloned as LF, to see if that's the only issue.

Elara6331 commented 2 years ago

The LF repo got a perfect build first try with no errors or modifications. As always, Windows ruins everything and crushes everyone's hopes and dreams.

agittins commented 2 years ago

The LF repo got a perfect build first try with no errors or modifications. As always, Windows ruins everything and crushes everyone's hopes and dreams.

Indeed! 🤣

So is there a specific change to .gitattributes that make it work for you? Should we replace the entire existing .gitattributes with:

* text=auto eol=lf
*.{cmd,[cC][mM][dD]} text eol=crlf
*.{bat,[bB][aA][tT]} text eol=crlf

or did you test with something else?

@Avamander I notice that you committed the original .gitattributes, in f69275484 was that to address some specific issues or was it a generic solution to PR's coming from windows machines, or specifically to support windows building?

Elara6331 commented 2 years ago

So is there a specific change to .gitattributes that make it work for you?

I cloned the repo with LF endings by setting core.eol=lf globally. I couldn't use .gitattributes as that only appears to work when the repo is cloned with the file.

agittins commented 2 years ago

I cloned the repo with LF endings by setting core.eol=lf globally. I couldn't use .gitattributes as that only appears to work when the repo is cloned with the file.

🤦🏼‍♀️ Oh yeah, of course! It looks like the snippet should work then, but some wider guidance might be required. I'll push it anyway that way you can test it directly and it can go up for review.

Elara6331 commented 2 years ago

It clones with LF now, so that should work

Elara6331 commented 2 years ago

I saw this in your commit:

# TODO: Do the rest of these entries need to be here? Does the eol=lf
# option defeat these anyway, or break anything? The binary ones seem
# superfluous, but maybe there is relevant context.

The reason for the binary entries is so that git leaves binary files alone, because otherwise, it will go through and replace CRLF with LF in binaries.

JF002 commented 2 years ago

@agittins

Hmm. I'd totally be all for just enforcing non-daft-line-endings, but it just depends on whether building on bare windows is considered to be a "supported" or desired build environment.

If building in docker is decreed as the only truly "supported" environment [...]

My main dev environment is "native" : I install the toolchain, SDK and other tools (font and picture converter, mcuboot image generator, dfu tools,...) on my system and build from there. I also use the Docker container mainly to build the firmware for the releases. I however do not use VSCode, devcontainer, MacOS and Windows so I can't maintain the support for them myself, but I'm OK if the community is willing to do it.

Elara6331 commented 2 years ago

@JF002 Building natively will still work on Linux and macOS. The issue is with the fact that Windows uses CRLF and the Linux container that VSCode/Docker starts uses LF. Since both macOS and Linux use LF anyway, it will work fine there. The only options that exist here are supporting all environments except Windows native specifically (which I believe doesn't work anyway), or forcing users to change the git clone command specifically if they're using Windows and want to build with a container.

Avamander commented 2 years ago

I notice that you committed the original .gitattributes, in f692754 was that to address some specific issues or was it a generic solution to PR's coming from windows machines, or specifically to support windows building?

Generic solution.

In general I would not break the possibility of having a native build environment on Windows. The container could do the clone if the directory is empty.

Elara6331 commented 2 years ago

Generic solution.

In general I would not break the possibility of having a native build environment on Windows. The container could do the clone if the directory is empty.

The way I understand it, devcontainers cannot do clones, because you can only start one after you open the repo in vscode. That would work for just the normal docker container, but not the devcontainer.

Avamander commented 2 years ago

@Arsen6331

Maybe the devcontainer could stand in a different repo that clones InfiniTime's?

Elara6331 commented 2 years ago

That doesn't seem like it would work too well, since devcontainers seem to do things like look for recommmended extensions in the source. 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).

agittins commented 2 years ago

Ok, I feel like we're drifting off-topic / out-of-scope somewhat.. can we back up a bit?

Up until now (afaik) bare-metal windows has not been a viable build environment. Nothing we do can possibly "break" or "preclude" building on windows, as the requirements are an unknown-unknown - and anything can be fixed later to support that.

Microsoft (who own vscode) are more and more embracing the use of linux runtime environments for different things, so I think bare-metal builds on windows are probably on the decline (just as they are in linux, on the whole), and it feels to me like MS see dockerised build environments as a good solution.

What this PR offers is a dead-simple way to develop, build and release full builds of infinitime, that works out-of-the-box on Linux, MacOS and Windows using free tools, and doesn't break other build envs in the process. This IS a (the?) solution to building on windows.

If a person has a yearning need to build on bare-metal windows, we are not doing anything to stop them from doing that, and if they find a way to do it that doesn't break anything else I'm sure their solution can be accepted into the repo. But us (well, I can only speak for myself, really) spending time right now trying to predict those requirements is not productive.

Let's have the repo check out in a state that works by default for every build environment that works. As of now that's Linux plus every platform on which devcontainers works. Feel free to open a PR that makes windows non-devcontainer builds work, but I feel like that's out of scope here.

I'll leave the generic text/binary definitions in the .gitattributes since they seem fairly benign, along with a note that those wanting to build bare-metal windows might try checking out the repo with --config core.eol=crlf for starters. IMO this is plenty enough, especially since we don't give specific instructions for other esoteric encodings like EBCDIC. PRs welcome though, I am sure! 😄

To summarise my lengthy, curmudgeonly diatribe:

agittins commented 2 years ago

Just looking at the docs and I see that vscode native on linux is a setup that at least used to work. I'll need to spend a bit of time to ensure my changes don't break that. I wonder how much of buld.sh should really be done in cmake somehow?

Avamander commented 2 years ago

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

This sounds like a better suggestion for Windows + Devcontainer users than modifying gitattributes to break any potential native editing/building.

agittins commented 2 years ago

@JF002

If building in docker is decreed as the only truly "supported" environment [...]

My main dev environment is "native"

Ahh, I was unclear - I was referring to Windows specifically - if what we "support" (if at all) on windows is the docker environment, then we just go for LF line-endings across the board, which means no change for Linux and MacOS users, but gives windows users an easy build option.

agittins commented 1 year ago

Just tagging PR's #1497 #1587