Skip to content

VSCode Devcontainer#505

Merged
JF002 merged 11 commits intoInfiniTimeOrg:developfrom
geekbozu:devcon
Aug 10, 2021
Merged

VSCode Devcontainer#505
JF002 merged 11 commits intoInfiniTimeOrg:developfrom
geekbozu:devcon

Conversation

@geekbozu
Copy link
Member

@geekbozu geekbozu commented Jul 20, 2021

Rehash of #219 using his docker container,
Applied a handful of updates to make it work with the current codebase/names
"fixed" 2 of the issues below and added a readme as well

Further work TBD

  • There is still an issue with running bash scripts in the Win10 instance due to the classic ^M problem. dos2unix is part of the container but permissions are getting in the way
  • clang-tidy and clang-format are part of the image but are not being invoke using VSCode
  • The project is not using the VSCode cmake extension
  • Flashing the target through OpenOCD/J-Link from within the container

The ^M problem is not something we really need to worry about...its a git/line ending setting. Git should be checking out with UNIX line ending when working with Vscode as it understands them...

No change to clang support maybe eventually....

We now use the cmake extension to handle build configuration AFTER the initial configuration. This leverages CMakesCache.txt

Instructions on how to flash/debug with OpenOCD remotely and a launch config for such have been added

The only "glaring" issue I know of at this point is a WSL2 issue on windows where windows filesystem access is slow. This can be gotten around by cloning the repo in the WSL2 machine and building it from there if so desired, this how ever does not effect it from working its just a nuisance worth mentioning.

@geekbozu
Copy link
Member Author

Regarding the WSL2 shenanigans, Some adjustments were made to allow us to use the"Clone in container" option of VSCode, Works way way nicer. ReadMe has been updated to reflect the steps for building in there.

@JF002 JF002 added this to the Version 1.4 milestone Jul 24, 2021
@geekbozu
Copy link
Member Author

I think is ready for some testing. I do not have a Linux box to test it without using the devcontainer... so some help there would be appreciated + some documentation review. I need to setup Launch Commands in the launch.json for non devcontainer stuff but again no way to test it...

@JF002
Copy link
Collaborator

JF002 commented Aug 4, 2021

@geekbozu is this PR ready to be merged? Also, does this PR replace #219? Or do we need to merge #219 in addition to this one?

@geekbozu
Copy link
Member Author

geekbozu commented Aug 5, 2021

I am still waiting for a 2nd developer to test it... as far as I know the devcontainer and supporting elements are done...Except clang support.
It replaces/Extends #219 I pulled his changes into my fork.
Let me have another day or 2 and I will try to get clang/clang tidy setup as the default formatters then this will be ready.
The stuff to adjust VSCode as a non containerized IDE should be a separate PR anyway.

@Riksu9000
Copy link
Contributor

It replaces/Extends #219 I pulled his changes into my fork.

This is similar to the conversation that happened in #544, but here the original author isn't credited.

@geekbozu
Copy link
Member Author

geekbozu commented Aug 6, 2021

It replaces/Extends #219 I pulled his changes into my fork.

This is similar to the conversation that happened in #544, but here the original author isn't credited.

Not going to lie but that's kind of hurt, The first line of this PR calls out the original authors PR. You do great work for this repo and I applaud the effort, but the first commit also points to his PR, I point it out in the first commit. He is credited, I didn't tag @nscooling just mentioning his PR did that. So instead of playing semantics maybe test it and let me know if it works for you :)

@Riksu9000
Copy link
Contributor

It replaces/Extends #219 I pulled his changes into my fork.

This is similar to the conversation that happened in #544, but here the original author isn't credited.

Not going to lie but that's kind of hurt, The first line of this PR calls out the original authors PR. You do great work for this repo and I applaud the effort, but the first commit also points to his PR, I point it out in the first commit. He is credited, I didn't tag @nscooling just mentioning his PR did that. So instead of playing semantics maybe test it and let me know if it works for you :)

Sorry, I didn't mean to upset you. I definitely don't think that you improving the code is wrong, and this is a very similar situation as what happened in my PR. What I meant was that the commits made by the original author don't exist here, so git thinks you made the changes. I think this can still be fixed with git so there won't be any issues, but someone had to mention it so this doesn't become a bigger issue.

@Avamander
Copy link
Collaborator

If you rebase on top of #219 then the original commits will be kept, which would be ideal when basing your work on something.

@geekbozu
Copy link
Member Author

geekbozu commented Aug 6, 2021

It replaces/Extends #219 I pulled his changes into my fork.

This is similar to the conversation that happened in #544, but here the original author isn't credited.

Not going to lie but that's kind of hurt, The first line of this PR calls out the original authors PR. You do great work for this repo and I applaud the effort, but the first commit also points to his PR, I point it out in the first commit. He is credited, I didn't tag @nscooling just mentioning his PR did that. So instead of playing semantics maybe test it and let me know if it works for you :)

Sorry, I didn't mean to upset you. I definitely don't think that you improving the code is wrong, and this is a very similar situation as what happened in my PR. What I meant was that the commits made by the original author don't exist here, so git thinks you made the changes. I think this can still be fixed with git so there won't be any issues, but someone had to mention it so this doesn't become a bigger issue.

And my lack of understanding git meant I fully did not understand where you were going. No worries! Just want to help contribute and be productive and it felt harsh when I really just have/had no idea how to attribute things to him properly! Thusly did it the way I knew how... :) I definitely want to attribute Niall where appropriate...just picked the only way I knew how! No hard feelings. Again keep up the awesome work!

If you rebase on top of #219 then the original commits will be kept, which would be ideal when basing your work on something.

I will give this a try and see if I can update this to do that then :) ... never touched the rebase button before so its a little daunting x.x

@geekbozu
Copy link
Member Author

geekbozu commented Aug 7, 2021

OK this updated Nialls stuff and then merged mine...I need to retest it all but it seems to have re-based properly...that was an experience....

@geekbozu
Copy link
Member Author

geekbozu commented Aug 9, 2021

Clang-tidy and Clang-format both now have plugins configured for them and are working in the container.
Container needed to be updated to use Clang-format 12 as it seems Clion uses some newer features then Clang-Format-10 which is the ubuntu default

@JF002
Copy link
Collaborator

JF002 commented Aug 10, 2021

@geekbozu Great work and thanks for rebasing your work on top of the previous one!
It would be nice if other developer tested this feature, but if no one is available right now, and if it works for you, we can still merge it. If someone finds an issue or an improvement, it will still be possible to improve it in the future :)

@geekbozu
Copy link
Member Author

At this point I am happy with that! It seems to work correctly on my 50th or so container rebuild..... so I'm going to guess it will work on 51+ as well :P
And thanks to everyone else for pointing me to how to...this git thing is tricky sometimes ;)

@JF002
Copy link
Collaborator

JF002 commented Aug 10, 2021

Thanks @geekbozu and everyone for this PR and review!
Feel free to open a new PR if you have any new feature or improvement to add to this feature :)

@JF002 JF002 merged commit 6430773 into InfiniTimeOrg:develop Aug 10, 2021
@eXeC64
Copy link

eXeC64 commented Aug 29, 2021

Thanks for this - it made it trivial for me to clone and get started with InfiniTime as a complete outsider to the project (though experienced with C, C++, and docker).

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.

7 participants