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

Update vscode configuration for devcontainers #1401

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

agittins
Copy link

@agittins agittins commented Oct 29, 2022

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:

  • de-duplicate build.sh, Dockerfile(s)
  • fix vscode creating invalid POSIX textfiles :-)
  • initial fixes for removing version fragility in vscode settings
  • updated config entries to match new cortex-debug extension
  • prevent built-in cmake from (invalidly) doing configure on open
  • reviews / feedback
  • update documentation to reflect any workflow changes: /doc/buildWithVScode.md, /.devcontainer/README.md
  • refinements of versioning / pathname fragilities (possibly in subsequent PR)
  • better integrate the config so that vscode can run cmake etc without breaking everything (again, probably a new PR)

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

@agittins
Copy link
Author

agittins commented Oct 29, 2022

Quick heads-up for anyone testing!

  • You need to have already done your git submodule update --init step
  • When you open vscode, it should ask you if you want to [ rebuild the devcontainer and] reopen in the devcontainer, say yes.
  • Once it has started the container and connected to it, rather than using the built-in run/build things, test by:
    • open the Terminal tab in the bottom pane
    • click the + "Launch profile" button (choose bash if it doesn't default to that)
    • run /opt/build.sh, it should do a full configure/make process, and give you a full set of files in your build dir.

image
image
image

- Added symlink creation to build.sh (and temporarily to
  .devcontainer/Dockerfile) for nRF5_SDK.
- Restored devcontainer envars for SDK, toolchain and sources.
- Improved some comments.
@Elara6331
Copy link
Contributor

Elara6331 commented Oct 29, 2022

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
Copy link
Author

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!

- didn't work on windows, so replacing with hard-coded version
  since the test will be bypassed once we have a newer build image.
@Elara6331
Copy link
Contributor

Elara6331 commented Oct 29, 2022

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
Copy link
Contributor

Elara6331 commented Oct 30, 2022

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

@Elara6331
Copy link
Contributor

Elara6331 commented Oct 30, 2022

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
Copy link
Author

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
Copy link
Contributor

Elara6331 commented Oct 30, 2022

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@Elara6331
Copy link
Contributor

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
Copy link
Author

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

@Elara6331
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

It clones with LF now, so that should work

@Elara6331
Copy link
Contributor

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
Copy link
Collaborator

JF002 commented Oct 30, 2022

@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
Copy link
Contributor

Elara6331 commented Oct 30, 2022

@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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

@Arsen6331

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

@Elara6331
Copy link
Contributor

Elara6331 commented Oct 30, 2022

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
Copy link
Author

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:

  • This PR does not break existing build systems
  • It fixes devcontainer builds so that they:
    • work without special checkout flag
    • work on Linux, MacOS, Windows
    • will continue to work as the main Dockerfile, build.sh and other toolchain parts are updated
  • It fixes the VSCode setup so that it:
    • works with the updated devcontainer configuration
    • builds (in devcontainer mode) on Linux, MacOS and Windows
  • Those features should be "enough" for this PR.

@agittins
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

@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
Copy link
Author

agittins commented Mar 7, 2023

Just tagging PR's #1497 #1587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants