-
Notifications
You must be signed in to change notification settings - Fork 11
Fix Arduino CLI: Upload
command
#4
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this major contribution, looks very well!
Please merge this repo's main branch into yours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good, thanks for the detailed description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's impressive! creating a docker file for easy build environment
for (const platform of platforms) { | ||
writeFileSync( | ||
customIgnoreFilePath, | ||
ignoreFile + `!assets/platform/${platform}/**` | ||
); | ||
mkdirSync(resolve(__dirname, "..", "out", "vsix"), { recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the output directory needs to be created inside of the loop, all VSIX packages are written to the same directory, I think that it is enough to create the directory once before the for loop.
@@ -323,7 +323,9 @@ | |||
] | |||
}, | |||
"scripts": { | |||
"dockerbuild": "docker build -t vscode-arduino-builder . && docker cp $(docker create vscode-arduino-builder):/app/out/vsix/. ./out", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NICE! including docker build command in the source, as a build option
@@ -1,12 +1,12 @@ | |||
{ | |||
"name": "vscode-arduino", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge this repo main branch into yours, because there are some modifications that I hade to do to this file in order to successfully upload the extension to marketplace
Hi, any update on merging this PR? |
This PR is sort of two changes, hopefully that's okay.
The first change, an actual bugfix:
Fix the command that gets run when running Arduino CLI: Upload.
There was an error that would appear:
error: unknown flag: --build-path
Previously, the command used the pure "upload" command from arduino-cli, which meant it wasn't compiling then uploading, just uploading.
This didn't match the Arduino: Upload command, which does compile, and separately, it seems it was intended to do the upload command because there was a --build-path flag being passed, which is for the compile command.
I changed the code so that instead of an arduino-cli upload command, it now does an arduino-cli compile --upload command, which does two things:
It makes it match the same functionality to the other Arduino: Upload command, and makes it so that it works with the flags that are passed in (those two facts make me relatively confident that was the intended purpose of this command in the first place)
It fixes the bug where there was an unknown flag, as that flag was for the arduino-cli compile command.
The second change was to make the development experience better:
There seemed to be a few parts of the codebase where there was some assumptions made about the developer's environment. For example, the correct version of node and npm, a specific version of python installed, g++, etc. Trying to build the project on an M1 Macbook was very challenging, I spent a couple hours trying to get it all working properly.
In the end, I had to update a number of dependencies (the old ones were deprecated anyways and everything seems to work properly still) as well.
To tie it all together, I wrote a Dockerfile which sets up the development environment properly and builds the project.
Finally, I added two commands to the package.json "scripts" section:
package which runs node ./build/package.js. This is just for convenience/discoverability since it took me a while to figure out how to even build the project.
dockerbuild, which builds the docker file and then runs the package script, and outputs the resulting .vsix files in the ./out directory.
I also updated the README as appropriate.
Fixes:
microsoft#1686
microsoft#1646
microsoft#1319