raspberrypi / pico-vscode

The official VS Code extension for Raspberry Pi Pico development. It includes several features to simplify project creation and deployment.
https://marketplace.visualstudio.com/items?itemName=raspberry-pi.raspberry-pi-pico
Mozilla Public License 2.0
121 stars 14 forks source link

Insert a single include(pico-vscode-sdk.x.x.x.cmake), instead of several lines in CMakes.list #39

Closed eriklundh closed 1 month ago

eriklundh commented 2 months ago

I bumped into this limitation when I generated a new project with the old sdk, and did not catch it until after some work had been done on the project.

The current approach to make a Pico project work with this plugin, inserts several lines enclosed in a warning:

== DO NEVER EDIT THE NEXT LINES for Raspberry Pi Pico VS Code Extension to work ==

==========================================================

That might have seen as a good idea at the time.

However, it does have some drawbacks:

  1. Obviously: The need for a stern warning at the start of that block of lines.
  2. Any further development of the VScode plugin where you might want to change parameters, eg test code with different SDK releases, would require the implementation a full parser of Cmake syntax, if you truly want a robust solution. Implementing and maintaining a full CMake parser might not be the best investment.

I strongly suggest that you move the necessary parameters to an include file, and also investigate if it is feasible to make other choices modular with CMake include files.

For the best tracking experince in Git or other VSCes of what happens in the project: Consider generating one file for e g SDK 1.5.1 (pico-vscode-sdk1.5.1.cmake) If the user choose that. Include that file, named with a suffix indicating the choice. Then if the user want to upgrade to SDK 2.0, you generate (or copy) another file named pico-vscode-sdk2.0.0.cmake This will make what has been done much more readable in the VCS log.

Note that it is a a less desireable approach to ad include files for all possible choics at project generation or import. That will make it harder to follow what has been done in the VCS log. Better to add (copy or generate) include files as needed.

will-v-pi commented 2 months ago

Yes, I've been thinking something like this would be sensible, although haven't implemented it yet - I was thinking it would take the form of a common pico-vscode.cmake file stored at .pico-vscode/cmake/ (rather than individual ones per project).

These would contain most of that section, and then the CMakeLists.txt file will only need to set PICO_SDK_VERSION, PICO_TOOLCHAIN_PATH, and PICOTOOL_VERSION before including the relevant file. We would still leave the warning around these few lines, but they would now only contain some version settings before including this additional cmake file.

will-v-pi commented 2 months ago

I've opened a PR #41 to prototype this - you're welcome to give that a try, you can download the .vsix from the actions run https://github.com/raspberrypi/pico-vscode/actions/runs/10404621655

eriklundh commented 2 months ago

Great to hear that you have been thinking along the same lines! May I suggest that you look into moving the remaining os/installation dependent tool paths and version info to the include file? That way a user on Windows can upload a generated project on e g Github, another user running e g Mac or Pi can download the project, and the plugin can much easier detect and suggest that the included file should be replaced with an equally named file that reflects the current dev environment? It might also be possible to offer to add that platform-specific include file to the .gitignore file, and have the plugin check for the include is missing and offer to generate a platform-specific replacement.

eriklundh commented 2 months ago

Yes, I've been thinking something like this would be sensible, although haven't implemented it yet - I was thinking it would take the form of a common pico-vscode.cmake file stored at .pico-vscode/cmake/ (rather than individual ones per project).

For an informed decision on placement of the include file, you might want to ask around in the Pico tools team (kilograham?) about the rationale for the design choice to add pico_sdk_import.cmake to every project. My gut feeling is that it was because of some caveats with CMake, but it could also be what is now cargo cult from the time when everything needed to come together in the tool chain.

will-v-pi commented 2 months ago

May I suggest that you look into moving the remaining os/installation dependent tool paths and version info to the include file? That way a user on Windows can upload a generated project on e g Github, another user running e g Mac or Pi can download the project, and the plugin can much easier detect and suggest that the included file should be replaced with an equally named file that reflects the current dev environment?

This should already be possible without these changes - there are no OS specific things in the CMakeLists.txt file, and the extension will automatically download any missing tools or a missing SDK version (the only exception being the Risc-V toolchain on Raspberry Pi, which is different to all other operating systems). The version info should be included in the project CMakeLists.txt file, to ensure that anyone else downloading the project will have the same versions as the person who made it.

There are some slightly non cross-platform things in the .vscode folder which we're sorting, but the CMakeLists.txt stuff should already be cross-platform.

For an informed decision on placement of the include file, you might want to ask around in the Pico tools team (kilograham?) about the rationale for the design choice to add pico_sdk_import.cmake to every project. My gut feeling is that it was because of some caveats with CMake, but it could also be what is now cargo cult from the time when everything needed to come together in the tool chain.

It's not required, as you can include it from ${PICO_SDK_PATH}/external/pico_sdk_import.cmake, but copying it into every project directory currently provides an easy way for the extension to detect whether a given folder is a pico project, and therefore can offer to import it. So we will stick with copying it into the project directory.

will-v-pi commented 1 month ago

Closing as this has been merged now

eriklundh commented 1 month ago

Well, the issues I wanted to address with my suggestion was that several lines of autogenerated Cmake code in the middle of a file, CMakeLists.txt, is a brittle construct, and using the plugin gui to change settings will result in a less obvious vcs log. Now with 0.15.2 we still have a brittle construct, and no improvement of how plugin gui changes will appear in the vcs log. What we got with 0.15.2 is still several lines enclosed in warnings - with just the non-varying lines moved to an insert file located in a common directory for all projects, i e an additional out-of-project dependency. This is almost the opposite of what I suggested, and I confess that I fail to see the point of making this change. It does not address any of the issues I raised.

will-v-pi commented 1 month ago

Apologies for misunderstanding - we use those lines to provide us a region of the CMakeLists.txt file which we can add configuration commands to. The only requirement on their placement is that they appear before include(pico_sdk_import.cmake) and after cmake_minimum_required, so developers are welcome to move them around. Abstracting most of the cmake configuration away to a separate out-of-project dependency makes it simpler to update it to add new functionality. Those lines also provide a simple way to detect whether projects are designed to work with the extension, so we will be keeping those lines.

Any changes to tool versions made in the GUI (ninja, CMake, SDK, toolchain, etc) will change either the CMakeLists.txt file, or the .vscode/settings.json file. It would then be up to the developer to make sure they commit those changes to VCS in a readable way.