Skip to content
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

devtool-ide fixes #2

Closed
wants to merge 13 commits into from
Closed

devtool-ide fixes #2

wants to merge 13 commits into from

Conversation

deribaucourt
Copy link
Member

No description provided.

afreof

This comment was marked as outdated.

scripts/lib/devtool/ide.py Outdated Show resolved Hide resolved
scripts/lib/devtool/ide.py Outdated Show resolved Hide resolved
scripts/lib/devtool/ide.py Outdated Show resolved Hide resolved
@deribaucourt deribaucourt force-pushed the ide-fixes branch 2 times, most recently from a02e63b to ec2e567 Compare October 4, 2023 13:22
scripts/lib/devtool/ide.py Outdated Show resolved Hide resolved
@deribaucourt deribaucourt force-pushed the ide-fixes branch 6 times, most recently from 39e96b1 to 57beb87 Compare October 5, 2023 10:19
On VSCode, when running the debugger through launch.json, the program is
recompiled, gdbserver is started, and connected to. However, the task to
install the program on the target is not run, so the program is not
updated on the target.

Adding a dependency to the `install && deploy-target` task fixes this.
Most packages do not provide a .editorconfig file. In those cases, the
recommended VSCode extension EditorConfig has no effect. Since devtool
ide is not producing a .editorconfig, it is not within our scope to
recommend this extension.
Just like CMake, Meson is usually used for C++ projects. We also
generate a c_cpp_properties.json file for Meson projects, so we should
recommend the C++ extension for VSCode.
In VSCode, the CMake extension must be configured for the intelliSense
engine to work. The user could already manually start this configuration
through the command palette.
@deribaucourt deribaucourt force-pushed the ide-fixes branch 2 times, most recently from 96483d8 to 902ffa9 Compare October 5, 2023 12:33
@deribaucourt deribaucourt marked this pull request as ready for review October 6, 2023 15:01
@@ -2328,12 +2328,11 @@ def test_devtool_ide_recipe_cmake(self):

# Verify re-building and testing works again
result = runCmd('%s --build --preset %s --target clean' % (cmake_exe, preset_name), cwd=tempdir)
self.assertIn("Cleaning all built files...", result.output)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's the issue. I just rebased my patches to the latest master and the tests pass without this patch here.

Copy link
Member Author

@deribaucourt deribaucourt Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my end I confirm that the cmake commands use a different output on my seutp.I'm getting:

2023-10-09 10:16:57,130 - oe-selftest - INFO - Traceback (most recent call last):
  File "/home/deribaucourt/Workspace/yocto-vscode/yocto/yocto-build/sources/poky/meta/lib/oeqa/selftest/cases/devtool.py", line 2337, in test_devtool_ide_recipe_cmake
    self.assertIn("Cleaning all built files...", result.output)
AssertionError: 'Cleaning all built files...' not found in "Change Dir: '/home/deribaucourt/Workspace/yocto-vscode/yocto/yocto-build/build-st/tmp/work/core2-64-poky-linux/cmake-example/1.0/cmake-example-1.0'\n\nRun Build Command(s): ninja -v clean\n[1/1] ninja  -t clean \nCleaning... 8 files."

This part especially looks different:

Run Build Command(s): ninja -v clean
[1/1] ninja  -t clean
Cleaning... 8 files.

My workspace has plain poky nanbield, with

  • cmake: 3.27.5
  • ninja: 1.11.1

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are changed more or less as you suggested. The patches are rebased to todays master and they pass.

@@ -2212,7 +2212,7 @@ def __devtool_ide_recipe(self, recipe_name, build_file, testimage):
conf_lines = [
'IMAGE_CLASSES += "image-combined-dbg"',
'IMAGE_GEN_DEBUGFS = "1"',
'IMAGE_INSTALL += "gdbserver %s-ptest"' % recipe_name
'PACKAGE_INSTALL:append = " gdbserver %s-ptest"\n' % recipe_name
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what't the issue here:

  • Using the += operator as mentioned in the commit messages was fixed long time ago. The commit message looks outdated.
  • Using PACKAGE_INSTALL instead of IMAGE_INSTALL add gdbserver also to special images like initramfs or the test-empty-image. This is probably not good.

I guess this commit is not needed.

Copy link
Member Author

@deribaucourt deribaucourt Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selftests were not passing on my nambield workspace because oe-selftest-image.bb overwrites IMAGE_INSTALL:
IMAGE_INSTALL = "packagegroup-core-boot packagegroup-core-ssh-dropbear libudev". Hence, the original IMAGE_INSTALL+= statement was overshadowed and the packages not present on target. I don't understand how the test could work on your end however. Maybe you had modifications to the selftest image or core images?
I agree that PACKAGE_INSTALL is less pedantic. The later patchset uses core-image-minimal to stick to what's done in other runqemu SSH selftests.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looks like you are running a pretty old version of my patches. This has been fixed several weeks ago.

  • v6 is with append. It's from one month ago.
  • v5 has the bug

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sue, here is the latest from today Oct 9 2023: https://git.yoctoproject.org/poky-contrib/log/?h=adrianf/devtool-ide

EXTRA_IMAGE_FEATURES += "ssh-server-openssh"
EXTRA_IMAGE_FEATURES += "debug-tweaks"

IMAGE_GEN_DEBUGFS = "1"
Copy link

@afreof afreof Oct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My setup looks like that:

# Build the companion debug file system
IMAGE_GEN_DEBUGFS = "1"
# But with devtool ide the dbg tar is not needed
IMAGE_FSTYPES_DEBUGFS = ""

# ssh is mandatory, no password simplifies the usage
EXTRA_IMAGE_FEATURES += "\
    ssh-server-openssh \
    debug-tweaks \
"

# Remote debugging needs the gdbserver on the target device
IMAGE_INSTALL:append = " gdbserver"

# Remote debugging works much better if the binaries are in the rootfs as well.
IMAGE_CLASSES += "image-combined-dbg"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I added the useful comments as well and removed the tar DEBUGFS.
Could you elaborate on what's not working well without image-combined-dbg? The debug symbols are not found when cross debugging?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes if I remember correctly, gdb needs the binaries as well to find some debug symbols.

I have now revised the doc patch. I hope now everything is included and it is better understandable. Also the shared sysroot and cmake-kit part should now be covered.

@@ -1074,6 +1077,10 @@ def setup_ide(self, args, image, gdb_cross):
if args.ide == 'none' and self.build_tool == BuildTool.CMAKE:
self.none_launch(image, gdb_cross)

if(image.gdbserver_missing):
logger.warning(
"gdbserver not installed in image. Remote debugging will not be available")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far this plugin is used for formatting : https://github.com/microsoft/vscode-autopep8. This patch introduces 2 little issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint, I ran the extension and fixed formatting.

The VSCode debugging configuration launches gdbserver on the target.
Warn the user if gdbserver is not installed in the image.
This reproduces the behavior without IMAGE_GEN_DEBUGFS.
`devtool ide` requires specific packages and image configurations to be
installed. Document them in the SDK manual. devtool will also produce
warnings if the user tries to run `devtool ide` without the necessary
dependencies.
Running the test with ctest simply checks the return value of the
program.
The file was overwritten unlike .vscode/*.json files which were merged
with what the original recipe provided, or user modifications.
However, the python dict.update() function does not merge nested
dictionaries, so some configurations will still be lost.
It seems that CMake logs with Yocto nambield are different. oe-selftest
ensure that the commands are run by looking for specific words in the
CMake output. I updated the test strings to match the new ones.
The IMAGE_INSTALL variable is now overriden in the image recipe
(oe-selftest-image.bb). We now use core-image-minimal as the base
like is done in othe SSH selftests like gcc.py and glibc.py.
This function is never called. The IntelliSense mode is configured
through configProvider in the c_cpp_properties.json file for CMake and
Meson classes.

This code could be usefull for future classes, but it is dead code for
now.
selftest/gcc.py and glibc.py provide great examples on how to make
qemu testing more reliable, notably on headless machines. This patch
applies the same logic to the devtool tests.
With the cmake-example recipe, the oe-local-files directory is
actually used when browsing or debugging the source code. The reason
is that VSCode automatically dereferences the symlinks present in the
workspace. Since the user will often be opening the symlinks and the
hard files, we should let him freely navigate them.
@afreof
Copy link

afreof commented Oct 9, 2023

I pushed an update to my branch here: https://git.yoctoproject.org/poky-contrib/log/?h=adrianf/devtool-ide

Changes:

  • Some of your findings I just fixed in my code (the trivial stuff)
    • remove editorconfig in my code
    • fix recommend vscode cpp extension for meson
  • 6 Patches are cherry-picked
  • Added 5 new commits
    • Added a fallback mode for recipes where we do not have native support like for cmake or meson
    • Make the arg parser smarter to handle e.g. something like that in one go: devtool ide cmake-example core-image-minimale meson-example
    • autotools support: needs much more testing
  • Docs: restructured and improved the shared sysroots mode
  • Tests: added a test for the shared sysroots mode

I think we could send a new version of the patchset including your patches. The last 5 from me are not ready yet.

@deribaucourt
Copy link
Member Author

including your patches. The last 5 from me are not ready yet.

Yes, that sounds like a great plan! Thanks again for reviewing my patches and updating them to your newest branch.

I had a quick glance at your 5 WIP commits, they look great!

  • I was wondering if we shouldn't add an abstraction for IDEs and recipe classes. As we add more classes like autotools, the ide.py file will start to get quite big, we could refactor it at some point into smaller units of code, each dedicated to an IDE or a bbclass while inheriting a common interface.
  • For the autotools class, I don't know if the configurationProvider passes compiler flags (like defines, additional include paths...). Make sure to test if that works. If you already did, you can ignore my message...

@afreof
Copy link

afreof commented Oct 11, 2023

  • I was wondering if we shouldn't add an abstraction for IDEs and recipe classes.

Yes we should! It's not at the top at my proiority list but it is something which is needed.

  • If you already did, you can ignore my message...

No I did not test it yet. I wrote the generator and run it. Then I improved my first blind try of the generator based on hints from the command line completion of VSCode. So yes, that would be helpful.

  • Yes, that sounds like a great plan!

Maybe a more concrete proposal for a plan. My assumption is that it will not be merged to poky master before end of month because poky is in feature freeze until then. So we should take the time to fix some details and bugs until then. My priority list:

  • Imporve the parser (patch is ready for testing, changes something from a user's perspective)
  • Fallback to devtool build and devtool deploy-target for recipes which are not natively support. (patch ready for testing, covers so many not handled cases)
  • Handle "-native" recipes. I did not test that at all so far, but it's obvious that e.g. the remote debug configuration does not make any sense in the native context. Ideally we add check which terminates devtool ide with a reasonable error message for such nativesdk- or -native of recipes.
  • Support multiple architectures. Use case: Change MACHINE variable and rebuild the IDE configuration would currently override the configuration for the old MACHINE. But that's not ideal. It would be better to add e.g. the PACKAGE_ARCH to the identifiers of the generated tasks and launch configurations.
  • Fix what your patch "devtool: ide: update CmakeUserPresets.json" tries to fix, but recursively

Later on:

  • Refactoring for modules IDE and modular Recipe support. This would also allow to e.g. add support for the clangd plugin to meta-clang or support for QTCreator to meta-qt
  • Support more languages (Python, Rust, Go)

Just let me know in case you would like to take one of the items or feel free to start with a refactoring for more modular design.

@deribaucourt
Copy link
Member Author

  • I was wondering if we shouldn't add an abstraction for IDEs and recipe classes.

Yes we should! It's not at the top at my proiority list but it is something which is needed.

  • If you already did, you can ignore my message...

No I did not test it yet. I wrote the generator and run it. Then I improved my first blind try of the generator based on hints from the command line completion of VSCode. So yes, that would be helpful.

  • Yes, that sounds like a great plan!

Maybe a more concrete proposal for a plan. My assumption is that it will not be merged to poky master before end of month because poky is in feature freeze until then. So we should take the time to fix some details and bugs until then. My priority list:

* Imporve the parser (patch is ready for testing, changes something from a user's perspective)

* Fallback to devtool build and devtool deploy-target for recipes which are not natively support. (patch ready for testing, covers so many not handled cases)

* Handle "-native" recipes. I did not test that at all so far, but it's obvious that e.g. the remote debug configuration does not make any sense in the native context. Ideally we add check which terminates devtool ide with a reasonable error message for such nativesdk- or -native of recipes.

* Support multiple architectures. Use case: Change MACHINE variable and rebuild the IDE configuration would currently override the configuration for the old MACHINE. But that's not ideal. It would be better to add e.g. the PACKAGE_ARCH to the identifiers of the generated tasks and launch configurations.

* Fix what your patch "devtool: ide: update CmakeUserPresets.json" tries to fix, but recursively

Later on:

* Refactoring for modules IDE and modular Recipe support. This would also allow to e.g. add support for the clangd plugin to meta-clang or support for QTCreator to meta-qt

* Support more languages (Python, Rust, Go)

Just let me know in case you would like to take one of the items or feel free to start with a refactoring for more modular design.

Those features sound awesome! At the moment, I'm rather focused on the vscode-bitbake extension to call bitbake/devtool through the UI instead of terminals so I don't have the bandwidth to help with refactoring. Have you had feedback from the maintainers on merging the current patches? From my understanding, it would help them reviewing the patch list if we stick to the current feature set and make sure the code is easy to read. The CMake integration has now been thoroughly tested and improved by the both of us so I think it would be nice to first merge that. The other features you mention could get added in a second iteration.

The code is already nicely structured into functions, but if I could suggest anything, it would be to move around the code specific to VSCode and bbclasses in dedicated .py files/classes and have it separate from the common devtool ide logic. From the maintainers' eyes, it would mean it's going to be easier for the community to extend support to other IDEs.

BTW I had previously mentioned that the sysroot flags were not properly used in the cmake-example devtool ide workspace. This was actually a bug from the cmake extension which I've fixed in microsoft/vscode-cmake-tools#3363

@afreof
Copy link

afreof commented Oct 12, 2023

Have you had feedback from the maintainers on merging the current patches?

The tests passed on the Yocto AB and the patches are queued on the next branches from bootlin. Some simple patches are already on master.

From my understanding, it would help them reviewing the patch list if we stick to the current feature set

Yes, that's my goal. I will send it as a new version of the same series. There will be a changelog in comparison to v6.

if I could suggest anything, it would be to move around the code specific to VSCode and bbclasses in dedicated .py files/classes and have it separate from the common devtool ide logic.

Yes, there is room for improvement, I agree. But it's an "internal detail" which can be changed at any time without changing something from a user's perspective.

First priority is to make the user interface and the generated script namings more future proof. Without that the user has to delete the workspace and run devtool ide from scratch otherwise some old json files or scripts will not be properly replaced. If we would change the command line interface later on, also your VSCode plugin would no longer be compatible. We should really avoid that as much as we can.

BTW I had previously mentioned that the sysroot flags were not properly used in the cmake-example devtool ide workspace. This was actually a bug from the cmake extension which I've fixed in microsoft/vscode-cmake-tools#3363

Updated my setup to the version with your fix in the changelog. It works! Thank you.

deribaucourt pushed a commit that referenced this pull request Feb 15, 2024
[YOCTO #15162]

when doing devtool modify, sources are extracted into a devtool
temporary workdir. The main source is moved inside
build/workspace/sources/${BPN}/ and local files are moved inside
build/workspace/sources/${BPN}/oe-local-files. Secondary sources are
currently not handled and are lost.

Here is the output of devtool modify/build on bzip2 recipe:

NOTE: bzip2: compiling from external source tree <...>/build/workspace/sources/bzip2
ERROR: bzip2-1.0.8-r0 do_install_ptest_base: ExecutionError('<...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/temp/run.do_install_ptest_base.3368', 1, None, None)
ERROR: Logfile of failure stored in: <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/temp/log.do_install_ptest_base.3368
Log data follows:
| DEBUG: Executing shell function do_install_ptest_base
| NOTE: make -j 16 DESTDIR=<...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest install-ptest
| sed  -n '/^runtest:/,/^install-ptest:/{/^install-ptest:/!p}' \
|            ../../../../../../workspace/sources/bzip2/Makefile.am      > <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/Makefile
| cp ../../../../../../workspace/sources/bzip2/sample1.ref      <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/
| cp ../../../../../../workspace/sources/bzip2/sample2.ref      <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/
| cp ../../../../../../workspace/sources/bzip2/sample3.ref      <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/
| cp ../../../../../../workspace/sources/bzip2/sample1.bz2      <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/
| cp ../../../../../../workspace/sources/bzip2/sample2.bz2      <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/
| cp ../../../../../../workspace/sources/bzip2/sample3.bz2      <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/
| ln -s /usr/bin/bzip2          <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/bzip2
| cp: cannot stat '<...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/git/commons-compress': No such file or directory
| WARNING: <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/temp/run.do_install_ptest_base.3368:189 exit 1 from 'cp -r <...>/build/tmp/work/core2-64-poky-linux/bzip2/
1.0.8/git/commons-compress <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/image/usr/lib/bzip2/ptest/bzip2-tests/commons-compress'
| WARNING: Backtrace (BB generated script):
|       #1: do_install_ptest, <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/temp/run.do_install_ptest_base.3368, line 189
|       #2: do_install_ptest_base, <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/temp/run.do_install_ptest_base.3368, line 158
|       yoctoproject#3: main, <...>/build/tmp/work/core2-64-poky-linux/bzip2/1.0.8/temp/run.do_install_ptest_base.3368, line 226
ERROR: Task (<...>/poky/meta/recipes-extended/bzip2/bzip2_1.0.8.bb:do_install_ptest_base) failed with exit code '1'
NOTE: Tasks Summary: Attempted 776 tasks of which 765 didn't need to be rerun and 1 failed.

Summary: 1 task failed:
  <...>/poky/meta/recipes-extended/bzip2/bzip2_1.0.8.bb:do_install_ptest_base

externalsrc class modify SRC_URI to keep only:
* 'file', 'npmsw' and 'crate' sources
* url with type parameter matching 'kmeta' or 'git-dependency'

So by forcing to add type='git-dependency' on secondary sources, we
ensure that when building the recipe, the secondary sources can be
unpacked into WORKDIR.

This allows recipes containing several sources to be built under a
devtool context, but it has some limitations:
* user would not be able to generate patches for the secondary sources
* type="git-dependency" is added for secondary sources even on non git
  sources, so we may want to rename this parameter

(From OE-Core rev: cfd5ee890163a3d975093359016dda104e7b71df)

Signed-off-by: Julien Stephan <[email protected]>
Signed-off-by: Richard Purdie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants