Skip to content
This repository was archived by the owner on Oct 1, 2024. It is now read-only.

Autogenerate c_cpp_properties.json #1141

Closed

Conversation

iFreilicht
Copy link

@iFreilicht iFreilicht commented Nov 18, 2020

Fixes #438
Fixes #808
Fixes #969
Fixes #1139
Fixes #966

Potentially fixes other issues that stem from an incorrectly set up c_cpp_properties.json.

This PR adds an autogeneration mechanism to the extension. This way, library paths and macro definitions are added to c_cpp_properties.json automatically.

See the original PR #1107 for more info. It was rebased by @hlovdal

* Tried to identify most of the tasks
* Added beer money support option
* First injection of the compiler command parser and IntelliSense auto-configuration. Currently injected into "verify" only.
* Updated branch documentation to reflect the current state of this project
…e replication, better maintainablility and readability

* Added pre-build command to "upload using programmer" since it was (probably unintentional) missing there
* Notes where to run the auto-generation
…uto-configuration to be turned off

* Prepared the compiler parser code to be injected into "upload" and "upload using programmer" without overhead
* Updated intellisense.ts to use only "let" instead of "var" which is much safer
* Updated branch documentation
…figuration flag which can override the global flag
* Made compile command regex match more stringent
…(comment))

* Added some ideas how to implement a better serial monitor
* Implemented c_cpp_properties merging -> compiler analysis results are merged into existing configuration and will preserve configurations of different name than the vscode-studio default configuration name (currently "Arduino"). This opens up the possibility for users to write their own configurations without having to disable the autogeneration.
* Implemented "write on change" - `c_cpp_properties.json` will only be written if a new configuration has been detected.
@adiazulay
Copy link
Contributor

@hlovdal @iFreilicht I merged develop into this pr, but I don't have push access. Everything looks like it's working right with the CLI and is ready merge unless you have any pending commits.

@iFreilicht
Copy link
Author

I'd still like to fix those two small annotations you had. @hlovdal, can you give Adi collaborator rights as well, so he can push the new rebase? I'll do the fixes then and after that we can merge 👍

@adiazulay
Copy link
Contributor

I also opened a PR on the fork (hlovdal#1) , so you can just merge that too.

@hlovdal
Copy link
Contributor

hlovdal commented Dec 11, 2020

Please do not merge anything into develop yet.

In the previous rebase I did I made significant effort into making the rebased branch as identical as possible to the original branch, only with modifications like updating the commit reference in the commit message to 5fd7ef8 for instance, since the branch was moving forward quite a distance and I did not want any changes other than that.

In the rebase I am currently working on I have fixed the branch so that every single commit compile, lint and test correctly. This is required for bisect to work (and a good thing regardless). I have also done some general version history clean up like for instance one commit added

    public port: StrSetting = new StrSetting();
    public board: StrSetting = new StrSetting();
    public sketch: StrSetting = new StrSetting();
    public output: StrSetting = new StrSetting();
    ...

which later were changed to

    public port = new StrSetting();
    public board = new StrSetting();
    public sketch = new StrSetting();
    public output = new StrSetting();
    ...

so I modified the original commit to just use the latter version from the start. Both are identical from the compilers's point of view, but now the version history contains less back and forth noise.

So the final result for old and new rebase should be virtual identical, but with some difference in the commits the result is composed from.

The current obstacle I am working on is the update made by pull request 1017 which merged the update and uploadUsingProgrammer functions into one combined function with some changes while this branch has merged the update, uploadUsingProgrammer and verify functions into one combined function with some significant changes compared to the mainline branch. This very much does not automatically resolve itself by tools (and in practice not manual effort either). The solution is of course to split up in smaller commits, and I think I am almost done with that. So I expect to be finished soon, if it had not been for 1017 I would have been done earlier this week.

@hlovdal
Copy link
Contributor

hlovdal commented Dec 11, 2020

@adiazulay I have added you as collaborator. Feel free to make and push any changes and modifications to the branch (but do not merge to develop). You and @iFreilicht do not have to hold back new commits or modifications until I am done with the rebase, I will have no problem with picking up and adopting changes you push in the meantime.

(Althoug preferably do not rebase content older than commit e12aa69 (Updated log, ...))

@adiazulay
Copy link
Contributor

@hlovdal I merged instead of rebased the develop branch and resolved the issues with upload/upload with programmer/verify.
I'm going to squash this branch when I merge it, so history in develop is going to stay clean.

Merged develop into intellisense-autoconfig.rebased
@adiazulay
Copy link
Contributor

@iFreilicht my PR is merged, you should be able to push your changes now.
@hlovdal let me know if you still want to continue rebasing the branch before merge.

@hlovdal
Copy link
Contributor

hlovdal commented Dec 16, 2020

Yes I am still working on it but have been busy with other things the last couple of days. I managed to finish the imerge rebase operation in the weekend, so the major work and challenge is done. There are a few commits on near the top of the branch that does not compile and a few other things I need to clean up, but it is getting close.

@adiazulay
Copy link
Contributor

@iFreilicht do you think you'll be able to get the last couple of fixes in this week? I'm hoping to merge this by Friday.

Let me know if there anything I can do to help.

@hlovdal
Copy link
Contributor

hlovdal commented Jan 7, 2021

@adiazulay Friday is too soon, neither your nor my branch is quite ready yet.

Some update from me: before Christmas I was busy working with a couple of woodworking gift projects, I was away for several days and after returning I had some problem with the pc not unencrypting the disk on startup after an update (I had to change a luks.uuid value). But now my pc is working and I am back in business and have finished my rebased branch to a state where it can be reviewed.

The result of my rebased branch is very similar to your branch where you merged and made additional changes in one commit,
however there are some errors and issues with your merge branch when comparing with my rebased brach:


The change from
"programmer=" + selectProgrammer
to
"programmer=arduino:" + selectProgrammer
has been discarded/lost.


-        if (!await this.runPrePostBuildCommand(dc, env, "pre")) {
+        if (dc.prebuild && !await this.runPrePostBuildCommand(dc, env, "pre")) {
             return false;
         }

This is the wrong place to test the value of dc.prebuild. This is already tested inside runPrePostBuildCommand with the existing cmdline test (which ought to be inverted) which is the right place to test this. However the merge commit cb3b978 failed to put the last return true statement at the correct place at the end (like it was originally when Uli extended runPreBuildCommand into runPrePostBuildCommand in commit 209c9b7), so it returns undefined instead of true when dc.prebuild is falsy. I guess that might have been the motivation for modifying the test above to compensate for that?


CliUpload and CliUploadProgrammer has been added in src/arduino.ts, however the build function is never called with those values. I assume this is an oversight and that those should have been used in src/extension.ts?


A couple of "--porgrammer" instead of "--programmer".


Some spelling mistakes (including pre-existing) of "available".


To make comparing the two branches simpler I have created a branch intellisense-autoconfig.rebased.compare on top of intellisense-autoconfig.rebased with a few formatting/rename changes to make the branches more similar.

When starting on this second rebase I used the merge commit cb3b978 as reference, and to avoid a moving target I kept working towards that, so some of the changes added after that is missing but I am not sure about all of them and instead of guessing on if some of the things that is different is significant I would like to have you compare the branches and provide some feedback on the things that are different.

I have pushed the second rebase branch as intellisense-autoconfig.rebased2.2021-01-07_001.

@adiazulay
Copy link
Contributor

@hlovdal thanks for the update! I'll review your branch today or tomorrow. Friday was just a goal, but it's no problem waiting a bit longer until everything is ready.

Thanks for all your work on this.

@adiazulay
Copy link
Contributor

@hlovdal I looked through your branch today. Looks good, much neater than mine. I'll get working on integrating the commits after cb3b978.

The change from
"programmer=" + selectProgrammer
to
"programmer=arduino:" + selectProgrammer
has been discarded/lost.

We've made updates to how programmers are being handled, so instead of hardcoding arduino: it gets the vendor associated with the programmer. I'll make sure those changes work with the new branch.

You're right about dc.prebuild it was able to run no problem with the IDE, but would just hang with the CLI. I'll test your branch to make sure it works.

That's just an oversight on those commands

I'll check all the spelling again, thanks for catching those!

@adiazulay
Copy link
Contributor

@hlovdal I opened a new PR with your rebased branch (#1183). I pushed a couple of fixes for the using the CLI.

All your changes look good to me. I'll wait to hear from you before merging the new PR.

Thanks for all the hard work everyone!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.