microsoft / vscode-makefile-tools

MAKE integration in Visual Studio Code
Other
184 stars 55 forks source link

WIP - Fix out of tree build #517

Open drok opened 8 months ago

drok commented 8 months ago

out-of-tree builds allow multiple configurations to be built simultaneously from one source tree. Each build configuration exists in a separate builddir, and the srcdir contains no configuration, object files or build artifacts.

In an autotools project which uses automake, the Makefiles are object files, ie they are generated specifically for a particular configuration.

When building an autotools project out-of-tree, the workspace is the $srcdir, and does not contain a Makefile. Instead, the Makefile is in $makeDirectory which is the $builddir.

This commit cleans up the detection of makefile location that is done in the extension so that the correct makefile is found, even if it lives out-of-tree/out-of-workspace, has a name other than Makefile/makefile, an whether or not a file named Makefile also exists in the $srcdir.

WIP because something doesn't work and I don't know how to debug (HELP wanted):

The deactivation is not expected, it is a result of a $srcdir/Makefile not existing, but the debugger does not pause the Extension Development Host when the deactivate() method is called. Instead, the Extension Development Host quickly reloads before I have a chance to examine the callstack. The callstack is of course cleared when the Host reloads.

There must be a way to diagnose exactly the chain of calls that leads to deactivate() being called, but I don't know how, because the callstack is prematurely cleared.

Repro:

Expected: The extension will activate/deactivate based on existence of /tmp/amhello-a/Makefile, without regard to ~/amhello/Makefile

If you know how to get vscode to debug the extension with Extension Development Host and have it stop when receiving the deactivate() call, and prevent the Host from reloading, and thus clearing the stack trace, please tell me how.

Note: this WIP PR builds on top of PR #500. Without it, an autotools+automake project would be unable to complete a dry run, even if building in-tree.

drok commented 8 months ago

There must be a way to diagnose exactly the chain of calls that leads to deactivate() being called

I ended up screen-recording with OBS. I then played back the recording and paused on the frame where the stack-trace was briefly displayed.

The reason for the deactivate() call was that resetting the extension's state was triggering a reload, and the extension was deactivated as the extensionHost was unloading.

In package.json, the extension was configured to be activated only if the workspace contained a makefile, so following the reload without a makefile present, the extension was never activated.

I have changed the configuration to always activate the extension (activationEvents='*'), meaning the user's wishes are not second guessed. If they enable the extension, it will be activated.

With the extension activated, one can then select a configuration (in some other builddir) which contains a Makefile, and thus the fullFeatureSet is enabled.

andreeis commented 8 months ago

@drok , I am not sure activating always (on "*") is a good idea. We don't want Makefile Tools starting to do its by default workflows always for any kind of code base. Also, VSCode did not provide a way to activate on settings values (to be able to read and check on disk at activation time if a file exists at a path location defined in a setting... I wish that will be provided soon). So you can't activate based on "makefile.makefilePath" unfortunately.

But... how about we activate on "makefile.am" or any other autotools files? I didn't ramp up on this practice yet but you can pick up this idea. True, for projects building out of tree without autotools we would have to ask developers to write an empty makefile in the root in order to activate because I don't know what other files to look for.

I'm looking at the PR now. I just wanted to write the above ideas before I start.

andreeis commented 8 months ago

I reviewed briefly, did not manually test yet. I'll get back to looking closer at this after I see more comments and you also have a chance to look at the failed tests. Some could be simple baseline changes (we log a lot and with such a change we do more stuff and/or differently which manifests in what we log) but I think some show some possible bugs in the PR to look at. And either way, thank you so much for taking the time to investigate and work on improving the whole area with so many PRs and proposals.

drok commented 8 months ago

@drok , I am not sure activating always (on "*") is a good idea.

@andreeis, allow me to persuade you that "*" is a better idea than the presence of a file:

First, I should say, on second thought, I think "onStartupFinished" would be better than "*" because there is no need to delay vscode startup.

  1. The idea of a dummy makefile is non-workable, because most users don't read the documentation in enough detail to know this is expected. In fact, I don't think the dummy makefile technique is even mentioned currently. They have to learn this the hard way, as I did, by wasting half a day "reading the source code". Sure, it works if you already know the secret incantation, but for most people this is a :thumbsdown: experience.

  2. Imagine a serial browser; someone who routinely examines projects from github. Every time they open a new project, the extension has no useful functionality to offer. Prior to this change, they'd have to create a dummy makefile to trick the extension into activating, but only if they're already atuned to the secret ritual.

  3. A developer who works out of tree most of the time (ie, me). They have one or more build configurations already configured, and a clean source tree (no dummy makefile). The extension remains hidden, syntax highlighting does not work for any of the known builds. With makefile.settings already present, cacheConfiguration present, etc, this extension still wants me to do one more special move and create the dummy file.

The extension should by default do the useful work it's advertising it's able to do. If there is no makefile, there should be no workflows the extension can do, so it should do nothing, but be active, able to configure the project and build it.

When some build configurations exist, the extension should definately load the default configuration, or the last selected configuration, and load the cache that corresponds to it. The source code should appear highlighted correctly for the last selected config, even if there is no makefile in the workspace directory, because the relevant makefile is out-of-tree.

After loading the cached configuration, the extension absolutely should proceed and refresh that cache with the "workflow". The cache may have become outdated while the editor was not active. Is it more reasonable for the extension to have all the information needed to do proper syntax highlighting, and still not do it, for want of avoiding "the workflow" ? Why is "the workflow" becoming a straw man? The workflow is the one job this extension has.

I would agree that for a freshly cloned project, with no makefile present, the extension should not try to "./configure"; however, it should offer this option "Looks like this autotools can be configured in tree [Do it] [No thanks]?", with a possible default setting "makefile.configureFreshWorkspacesAutomatically ... [yes / no / ask]

As you said,

So you can't activate based on "makefile.makefilePath" unfortunately.

Since the one file that is the key to activation is not known to vscode, it follows that the extension must do the little bit of work of checking if there is useful work to be done or not, under all circumstances.

We don't want Makefile Tools starting to do its by default workflows always for any kind of code base.

Why not? What kind of workflows would it do if there is no makefile, no builds configuration, no instructions in the settings.json file? Would it not be a nop to activate it in a freshly cloned directory?

On the other hand, if the freshly cloned directory (the "any kind of code base" as you describe it) did include a Makefile, this would be trigger enough to "start the workflows"? So you can't say that the presence of a Makefile is any kind of safety trigger. Using it as an activation condition does not prevent the case you say it can prevent ("run the workflows on any kind of code base").

But... how about we activate on "makefile.am" or any other autotools files?

As long as the extension should be compatible with non-autotools projects that contain arbitrarily named makefiles (ie, makeifle.makefilePath), I can't see how it's workable to hardcode a fixed filename to look for. If the extension goal was to support autotools projects, then sure, the presence of the well-known autotools inputs would work.

I'm looking at the PR now. I just wanted to write the above ideas before I start.

I appreciate you looking at this PR in detail! Thank you!