-
-
Notifications
You must be signed in to change notification settings - Fork 952
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
base: main
Are you sure you want to change the base?
Update vscode configuration for devcontainers #1401
Conversation
- removed duplicated dockerfile and build.sh - misc small updates
Quick heads-up for anyone testing!
|
- 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.
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 Here's what I mean: This is after I ran My macOS system is still building the devcontainer, I'll get back to you with the results of that one. |
Huzzah! Thanks for trying it out!
Yay!
...
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. Oh wait... I think what's happening is that there's a bodgy step I'm doing in If it lets you into the container, can you try creating the two symlinks manually?
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 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!
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.
Yeah, there were just a bunch of bash syntax errors, and it also tried to use
You're probably right. Your latest commit fixed the script itself (no bash errors, even with un- 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.
Just saw that it's named with
It's compiling InfiniTime right now. Might take a while on its 2nd gen i5. |
MacOS finally finished building. No errors, everything worked great. It's just Windows and their |
After running Edit: Yep, looks like |
Great work chasing that down! Hmmm... at https://code.visualstudio.com/docs/devcontainers/tips-and-tricks it suggests adding to
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! |
No, I've just encountered another error involving windows CRLF. Specifically, something attempted to execute |
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. |
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 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. |
I am currently trying locally with a repo cloned as LF, to see if that's the only issue. |
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
or did you test with something else? @Avamander I notice that you committed the original |
I cloned the repo with LF endings by setting |
🤦🏼♀️ 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. |
It clones with LF now, so that should work |
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. |
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. |
@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. |
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. |
@Arsen6331 Maybe the devcontainer could stand in a different repo that clones InfiniTime's? |
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 Another option could be to instruct Windows users to clone with |
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 To summarise my lengthy, curmudgeonly diatribe:
|
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? |
This sounds like a better suggestion for Windows + Devcontainer users than modifying gitattributes to break any potential native editing/building. |
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. |
The existing vscode config (in
.vscode
and.devcontainers
) included duplication of the Dockerfile and build.sh fromdocker/
. 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 onebuild.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:
cortex-debug
extension/doc/buildWithVScode.md
,/.devcontainer/README.md
Feedback / review especially welcome from anyone who can test this on their system with vscode.