GNOME Bugzilla – Bug 790846
Some improvements to the JS code indexer
Last modified: 2017-12-04 00:23:37 UTC
It's great to have a gjs code indexer, and it just needs a little help to make it a nice node.js code indexer.
Created attachment 364418 [details] [review] gjs_symbols: don't choke on toplevel destructuring assingments A destructuring assignment Pattern does not have a 'name' property, which would crash the symbol extractor and abort everything.
Created attachment 364419 [details] [review] gjs_symbols: be compatible with CommonJS too Recognize the Common JS idioms: "const Foo = require(...)" to import a module "module.exports = class Foo { ... }" to declare the main class of a module
Created attachment 364420 [details] [review] gjs_symbols: don't ignore files in node_modules/ The code indexer already ignores all directories that are gitignored. If node_modules/ is not gitignored, it means it contains files that the developer cares about.
Review of attachment 364418 [details] [review]: LGTM
Review of attachment 364420 [details] [review]: LGTM
Review of attachment 364420 [details] [review]: > The code indexer already ignores all directories that are gitignored. If node_modules/ is not gitignored, it means it contains files that the developer cares about. I still expect this to lead to complaints. If you add the directory before ignoring it the indexer will pick it all up and no UI is exposed to easily clear it and some users might just not be familiar with git. As I'm sure you know indexing this directory can literally be indexing *hundreds* of projects and *thousands* of files. I'm just not sure that is something to do lightly?
Review of attachment 364419 [details] [review]: Excellent, thanks!
(In reply to Patrick Griffis (tingping) from comment #6) > Review of attachment 364420 [details] [review] [review]: > > If you add the directory before ignoring it the indexer will pick it all up > and no UI is exposed to easily clear it and some users might just not be > familiar with git. As I'm sure you know indexing this directory can > literally be indexing *hundreds* of projects and *thousands* of files. I'm > just not sure that is something to do lightly? I disagree with the point that people might unfamiliar with git. Developers have been using VCSs for decade, and all the good VCSs (all the way back to SVN) have some concept of ignoring files. I expect the users of Builder to understand that. On the other hand, I take your point that for brand new projects, close to the initial commit, node_modules/ might not be in gitignore yet. (Same thing if some modules are ignored and some aren't, and later you add a new module) If that happens, Builder will index those files, resulting in a lot of unremovable garbage in the index. Would it be a good idea instead to ignore both gitignored and untracked files? At a later time, if it turns out the new file is source and not dependency, the developer will add it to git and the indexer will pick it up.
Created attachment 364473 [details] [review] IdeRunner: don't override the CWD of the launcher It is set a few lines above by real_create_launcher()
Created attachment 364474 [details] [review] IdeBuildTarget: add get_cwd() Some build systems (notably, npm), insist that their build targets be run in a specific working directory, or they'll misbehave. To allow BuildTargets and BuildTargetProviders specify that, introduce a vfunc get_cwd(). If this vfunc returns any non-NULL value, IdeRuntime uses it to set the cwd of the created IdeRunner.
Created attachment 364475 [details] [review] npm: add BuildTargetProvider Which allows one to specify what to run in package.json, as the "start" script (a node.js convention), and then uses npm to run it.
Created attachment 364476 [details] [review] IdeBuildTarget: add get_language() Add a way to find the programming language of a build target. This will be used to weed out incompatible debuggers, profiling tools, etc. Cargo build targets are changed to return rust, NPM to return JS. Similarly, a future Cabal integration should say Haskell, Maven or ANT should say Java, etc. For now, automake, meson and cmake return "asm" (native code), because it's annoying to find if the build target is built from C, C++, Vala or Fortran, and it does not matter too much because gdb and valgrind can deal with it.
Created attachment 364477 [details] [review] GdbDebugger: chain up properly from dispose If you chain up twice to finalize, you're going to have a bad time.
Created attachment 364478 [details] [review] IdeDebugManager: check the programming language to see if the debugger is compatible Make it possible to retrieve the IdeBuildTarget from the IdeRunner, and use it to retrieve the programming language of the program being run. Compare it to the list of languages claimed by the debugger to see if the debugger is compatible. Tested with npm on host (correctly claims no debugger is available) and meson on flatpak (correctly starts gdb)
While we're here, I'm adding some more JS improvements (mostly nodejs related), in the area of running and debugging.
Review of attachment 364473 [details] [review]: sure
Review of attachment 364474 [details] [review]: couple real quick fixes, then fine to push ::: src/libide/buildsystem/ide-build-target.c @@ -125,0 +132,13 @@ + +/** + * ide_build_target_get_cwd: ... 10 more ... (nullable) @@ -125,0 +132,18 @@ + +/** + * ide_build_target_get_cwd: ... 15 more ... Remove extraneous space @@ +148,3 @@ +gchar * +ide_build_target_get_cwd (IdeBuildTarget *self) +{ g_return_val_if_fail (IDE_IS_BUILD_TARGET (self), NULL); ::: src/libide/runtimes/ide-runtime.c @@ -171,2 +171,3 @@ g_auto(GStrv) argv = NULL; const gchar *slash; + g_autofree gchar *cwd; We always initialize autoptr/autofree variables to NULL in the Builder codebase.
Review of attachment 364475 [details] [review]: Seems like a useful addition. Need to drop the use of lambda then fine to push. ::: src/plugins/npm/npm_plugin.py @@ -109,0 +111,44 @@ + +# The scripts used by the npm build system during build +# we don't want to include these as build target ... 41 more ... Would sort of prefer to have a variable defined than script[3:] repeated @@ +205,3 @@ + + project_file = context.get_project_file() + project_file.load_contents_async(cancellable, lambda *x: self._on_package_json_loaded(task, *x)) We don't use lambda in the Builder code-base because it sometimes causes runtime memory reference issues that are super hard to track down. So needs to be a callback function.
Review of attachment 364476 [details] [review]: Nice work, we can use this with sysprof once we get that spidermonkey profiler merged too. Couple small nits then good to push. ::: src/libide/buildsystem/ide-build-target.c @@ +182,3 @@ + */ +gchar * +ide_build_target_get_language (IdeBuildTarget *self) remove extra spacing @@ +184,3 @@ +ide_build_target_get_language (IdeBuildTarget *self) +{ + return IDE_BUILD_TARGET_GET_IFACE (self)->get_language (self); Add g_return_val_if_fail (IDE_IS_BUILD_TARGET (self), NULL); ::: src/libide/buildsystem/ide-build-target.h @@ +54,3 @@ gchar *ide_build_target_get_cwd (IdeBuildTarget *self); IDE_AVAILABLE_IN_3_28 +gchar *ide_build_target_get_language (IdeBuildTarget *self); Fix alignment
Review of attachment 364477 [details] [review]: Yuck. Thanks for finding this
Review of attachment 364478 [details] [review]: Looks like you're headed down the right path, couple fixes before pushing ::: src/libide/debugger/ide-debug-manager.c @@ +34,3 @@ #include "plugins/ide-extension-util.h" #include "runner/ide-runner.h" +#include "buildsystem/ide-build-target.h" Alphabetically sort @@ -702,0 +703,14 @@ +static gboolean +debugger_supports_language (PeasPluginInfo *plugin_info, + const gchar *language) ... 11 more ... We get to use modern C features for Builder, so for (guint i = @@ -719,0 +743,5 @@ + IdeBuildTarget *build_target = ide_runner_get_build_target (lookup->runner); + + if (build_target != NULL) ... 2 more ... * next to variable, not base-type, so "g_autofree gchar *language = ..." ::: src/libide/runner/ide-runner.c @@ +684,3 @@ + * that will be launched, such as what programming language it uses, + * or whether it's a graphical application, a command line tool or a test + * program. Since: 3.28 @@ +687,3 @@ + */ + properties [PROP_BUILD_TARGET] = + g_param_spec_boolean ("build-target", g_param_spec_object @@ +690,3 @@ + "Build Target", + "Build Target", + FALSE, IDE_TYPE_BUILD_TARGET @@ +1423,3 @@ + * + * Returns: (nullable) (transfer none): The %IdeBuildTarget associated with this %IdeRunner, or %NULL. + * See IdeRunner:build-target for details. #IdeRunner:build-target @@ +1455,3 @@ + + if (g_set_object (&priv->build_target, build_target)) + { No braces necessary ::: src/plugins/flatpak/gbp-flatpak-runtime.c @@ -286,3 +287,4 @@ binary_name = get_binary_name (self, build_target); - return IDE_RUNNER (gbp_flatpak_runner_new (context, build_path, binary_name)); + runner = IDE_RUNNER (gbp_flatpak_runner_new (context, build_path, binary_name)); + ide_runner_set_build_target (runner, build_target); if (build_target != NULL) ?
Comment on attachment 364477 [details] [review] GdbDebugger: chain up properly from dispose Attachment 364477 [details] pushed as c15bf32 - GdbDebugger: chain up properly from dispose
All comments were addressed. Attachment 364418 [details] pushed as 4b0cf06 - gjs_symbols: don't choke on toplevel destructuring assingments Attachment 364419 [details] pushed as 48af8a9 - gjs_symbols: be compatible with CommonJS too Attachment 364420 [details] pushed as 49511a8 - gjs_symbols: don't ignore files in node_modules/ Attachment 364473 [details] pushed as f1fdc4d - IdeRunner: don't override the CWD of the launcher Attachment 364474 [details] pushed as e891c08 - IdeBuildTarget: add get_cwd() Attachment 364475 [details] pushed as eea6aa8 - npm: add BuildTargetProvider Attachment 364476 [details] pushed as b2e1d33 - IdeBuildTarget: add get_language() Attachment 364478 [details] pushed as 81410f2 - IdeDebugManager: check the programming language to see if the debugger is compatible