Hi Ross Thank you for the re-view.
Summary: I hope everything is fixed with v7: https://lists.openembedded.org/g/openembedded-core/message/189812 On Thu, 2023-10-12 at 12:53 +0000, Ross Burton wrote: > Finally looking at the code… > > On 10 Sep 2023, at 16:52, Adrian Freihofer via lists.openembedded.org > <adrian.freihofer=gmail....@lists.openembedded.org> wrote: > > > > The new devtool ide plugin configures an IDE to work with the eSDK. > > > > With this initial implementation VSCode is the default IDE. > > The plugin works for recipes inheriting the cmake or the meson > > bbclass. > > Support for more programming languages and build tools may be added > > in > > the future. > > Can the vscode pieces be split out to a separate file, to separate > out the high level logic and the vscode specifics. I’m pretty sure > there would be interest in adding qtcreator, for example. By doing > this sooner rather than later we avoid adding cheeky “lets just > handle vscode specially” blocks. Also, splitting out the > cmake/meson/none logic where it isn’t IDE-specific. > > Also one thing I’d like to encourage more is decent code > documentation, to make the code easier to both review and work on in > the future. Could you add at least basic documentation to the > classes and major functions, with a summary of how the plugin works? > > > + def __gdbserver_start_cmd(self, binary, port): > > + if self.gdbserver_multi: > > + gdbserver_cmd = "/usr/bin/gdbserver --multi :%s" % ( > > + port) > > + else: > > + gdbserver_cmd = "/usr/bin/gdbserver --once :%s %s" % ( > > + port, binary) > > If I’m being pedantic, that should be ${bindir}. > > > + if 'debuginfod' in > > image_d.getVar('DISTRO_FEATURES').split(): > > + # image_config.debuginfod = True > > + logger.warning("Support for debuginfod is not > > implemented yet.") > > What doesn’t work, and why is this warning needed? I removed the warnings related to debuginfod. Currently the rootfs-dbg is used, but debuginfod should not harm and no warning is needed. > > > + def solib_search_path_rootfs(self): > > + """Search for folders with shared libraries in the rootfs > > and rootfs-dbg > > + > > + This is based on the assumption that the > > PACKAGE_DEBUG_SPLIT_STYLE variable from the image > > + is the global setting which is used by most packages. Even > > if this variable does not seem > > + to make sense in the image context. > > + """ > > + rootfs_solib_search_path = [] > > + rootfs_dbg_solib_search_path = [] > > + if self.package_debug_split_style in ['debug-with-srcpkg', > > '.debug']: > > + if self.combine_dbg_image: > > + rootfs_dbg_solib_search_path = [ > > + "/lib", "/lib/.debug", "/usr/lib", > > "/usr/lib/.debug"] > > + else: > > + logger.warn( > > + 'Adding IMAGE_CLASSES += "image-combined-dbg" > > offers better remote debugging experience.') > > + rootfs_solib_search_path = [ > > + "/lib", "/usr/lib"] > > + rootfs_dbg_solib_search_path = [ > > + "/lib/.debug", "/usr/lib/.debug"] > > + elif self.package_debug_split_style == 'debug-file- > > directory': > > + rootfs_dbg_solib_search_path = ["/usr/lib/debug"] > > + else: > > + logger.warning( > > + "Cannot find solib search path for a rootfs built > > with PACKAGE_DEBUG_SPLIT_STYLE=%s." % > > self.package_debug_split_style) > > + > > + sym_dirs = [] > > + for dbgdir in rootfs_solib_search_path: > > + sym_dirs.append(os.path.join( > > + self.rootfs, dbgdir.lstrip('/'))) > > + for dbgdir in rootfs_dbg_solib_search_path: > > + sym_dirs.append(os.path.join( > > + self.rootfs_dbg, dbgdir.lstrip('/'))) > > + > > + return sym_dirs > > Feels like there should be a much better way to do this. Does the > choice of debug split style actually matter? Just search all of the > candidates and avoid trying to special-case everything? It's probably still not as simple as you would expect. But it's much simpler because I dropped the combined-debug-dbg cases. I prefer to handle the different cases separately and provide a reasonable error message to the user. There are so many reasons why the debugger cannot find the symbols and sources. > > I’d not noticed image-combined-dbg existed and do wonder if that > shoud be the behaviour of the debug rootfs. Is there actually a use- > case for a tarball which is _just_ the symbols? > Yes, it seems best to remove the image-combined-dbg.bbclass and make it the default. There is now a patch that does this. This solves many problems when tools need debug symbols and sources. > > +class RecipeMetaIdeSupport: > > + """Handle some meta-ide-support recipe related properties""" > > I’m obviously missing something: what’s the use-case for using meta- > ide-support instead of the recipe’s depends? > devtool ide has two modes: 1. Generate an IDE configuration for a recipe in the workspace generated by devtool modify. For this mode we do not need meta-ide- support. Correct. 2. Generate an eSDK like behavior where the generic environment file can be sourced into a shell without any dependency on a recipe. devtool ide basically does what bitbake meta-ide-support build-sysroots does. In case the IDE is VSCode, it adds a cmake-kit to the users global VSCode configuration. This allows then to use the tool-chain provided by Yocto instead of the tool-chain from the host distro. This is also covered by the docs update. See *Shared sysroots mode*. Adrian > > + def solib_search_path_sysroot(self): > > + return [os.path.join(self.recipe_sysroot, p) for p in > > ['lib', 'usr/lib’]] > > ${base_libdir}, ${libdir}. Some systems use /usr/lib64 or /usr/lib32 > and this assumption will fail. > > Ross
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#189821): https://lists.openembedded.org/g/openembedded-core/message/189821 Mute This Topic: https://lists.openembedded.org/mt/101275530/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
