-
Notifications
You must be signed in to change notification settings - Fork 64
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
WIP - Fix out of tree build #517
base: main
Are you sure you want to change the base?
Conversation
'all' is a well-known name The default target must be explicitly named on the dry-run command line, because of [Remaking Makefiles](https://www.gnu.org/software/make/manual/html_node/Remaking-Makefiles.html) rule of GNU Make. In order to avoid remaking makefiles during a dry-run, the makefile must also be given as an explicit target. If the makefile were given as the only target, the dry-run would not mean 'dry-run the default target', but 'dry-run nothing'. Thus, if the makefile must be given as a target, the default target must be made explicit, and thus is must be defined. Also, previously 'all' was added to the make invokation for listing targets. This would prevent the .DEFAULT_GOAL to be correctly set as the Makefile declared, and instead be set to the value given on the command line. An explicit target is no longer given on the --print-data-base invocation, so the output .DEFAULT_GOAL will be the genuine default goal. This commit makes currentTarget always defined, with the default value 'all'; this is a reasonable, well-known default.
The top makefile in projects that use autotools and automake contains a rule to remake the makefile itself when the configuration changes (configure.ac). Even when dry-running, GNU make regenerates the makefile, in a bid to generate a 'correct' dry-run output. VScode needs to add --always-make in order to get a complete view of the dry-run. Without it, it would only get the commands needed for outdated targets. These two behaviours combined cause a naive 'make --dry-run --always-make' to continuously remake the Makefile. In order to avoid this infinite loop, make must be instructed as in the "Remaking Makefiles" man page, to avoid remaking the makefile. This is done by adding two options: --assume-old=Makefile, and Makefile (ie, target). Make requires the Makefile to be explicitly specified as target, otherwise it ignores the --assume-old=Makefile option. Furthermore, Makefiles generated by automake cause the top invocation to be a recursive sub-make invocation. On recursive makes, make itself calls submake without passing the --assume-old option, thus breaking the combo required for the sub-make to avoid remaking the makefile. As a result, automake Makefiles need one more workaround. The --assume-old option must be manually passed to sub-make via the AM_MAKEFLAGS, which is always appended to sub-make's command line. This commit implements the above incantation to enable automake Makefiles to be dry-run without an infinite loop. Additionally, the makefilePath, makeDirectory, updatedMakefilePath and updatedMakeDirectory variables are made to store the respective setting, rather then re-purposing them to store the corresponding resolved, absolute paths. makefilePath cannot be undefined because for dry-running the name of the makefile must always be supplied on the make commandline.
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. The builddir need not be a subdirectory of srcdir, and typically it is on another filesystem than the sources. In an autotools project which uses automake, the Makefiles are object files, ie they are generated specifically for a particular configuration, therefore they reside in the builddir. 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. The activationEvent is changed to "*" because when building out of tree, there are no makefiles in the workspace. "*" is the better activation because it means activate whenever the user enabled the extension; don't second guess. OTOH, the extension does nothing if a makefile is not found and not configured, so it's safe to have it activated on workspaces without makefiles, because of the cleaned-up detection/configuration of makefiles.
When building out of tree, whether the extension is in fullFeatureSet or not depends on which build configuration is chosen. The workspace does not have a Makefile, but one of the builddirs in a configuration does. Allow that configuration to be selected, unconditionally on the currently selected configuration.
619073e
to
37ee01c
Compare
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 In I have changed the configuration to always activate the extension ( With the extension activated, one can then select a configuration (in some other builddir) which contains a Makefile, and thus the fullFeatureSet is enabled. |
@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. 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. |
makeArgs.push(currentTarget); | ||
} | ||
} | ||
|
||
// Include all the make arguments defined in makefile.configurations.makeArgs | ||
makeArgs = makeArgs.concat(configuration.getConfigurationMakeArgs()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why you moved makeArgs.push(currentAll) to later below... but what about "all" in case of "forTargets"? You did not do that intentionally? When "forTargets" is true, we need a slightly different set of args passed to "make" and if not correct we don't identify correctly all available targets. We need "all" in that case and with your change I don't see where we make sure we add "all".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recall that in #217, "all" does not work for target discovery.
Removing "all" as an explicit target means make will work on the .DEFAULT_GOAL, whatever that happens to be (should be "all" in most or all autotools projects, but it can be something else, like "sub-make" in #217
This won't fix 217, because in that example the wrapper makefile has no ability to recurse, but it will avoid an unnecesary error ("target 'all' does not exist" in cases like #217). Adding 'all' to this command does nothing, as far as I can see.
Was "all" added for some specific purpose, where the implicit target was not better?
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. |
@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.
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,
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.
Why not? What kind of workflows would it do if there is no makefile, no builds configuration, no instructions in the 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").
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 appreciate you looking at this PR in detail! Thank you! |
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):
$srcdir
, the extensionfunctions correctly.
$srcdir
, but only in$builddir
(as is typical), the extension is asynchronously deactivatedafter doing all the dry-run work.
The deactivation is not expected, it is a result of a
$srcdir/Makefile
notexisting, but the debugger does not pause the Extension Development Host
when the
deactivate()
method is called. Instead, the ExtensionDevelopment 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:
/usr/share/doc/automake-1.13.4/amhello-1.0.tar.gz
) but missing on some Debian/Ubuntus.~/amhello
)./tmp/amhello-a
directory (this will be the build directory)/tmp/amhello-a
~/amhello/configure
in /tmp/amhello-a)touch ~/amhello/Makefile
/tmp/amhello-a
using the makefile/tmp/amhello-a/Makefile
rm ~/amhello/Makefile
touch ~/amhello
)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.