From a7dbf11bc0c3f86db213d199bf7df85a5404ab8a Mon Sep 17 00:00:00 2001 From: drok Date: Wed, 6 Sep 2023 15:09:06 -0500 Subject: [PATCH 1/2] Assume the default build target is 'all' '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. --- src/configuration.ts | 18 ++++++++---------- src/make.ts | 14 +++----------- src/test/fakeSuite/extension.test.ts | 8 -------- 3 files changed, 11 insertions(+), 29 deletions(-) diff --git a/src/configuration.ts b/src/configuration.ts index ed5b23f7..ae2208b4 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -936,24 +936,22 @@ export async function readMakefileConfigurations(): Promise { // Last target picked from the set of targets that are run by the makefiles // when building for the current configuration. // Saved into the settings storage. Also reflected in the configuration status bar button -let currentTarget: string | undefined; -export function getCurrentTarget(): string | undefined { return currentTarget; } -export function setCurrentTarget(target: string | undefined): void { currentTarget = target; } +// Assume the well-known "all" target for the default goal +let currentTarget: string = "all"; +export function getCurrentTarget(): string { return currentTarget; } +// Set the current target, or 'all' if none was given +export function setCurrentTarget(target: string = "all"): void { currentTarget = target; } // Read current target from workspace state, update status bar item function readCurrentTarget(): void { let buildTarget : string | undefined = extension.getState().buildTarget; + setCurrentTarget(buildTarget); if (!buildTarget) { - logger.message("No target defined in the workspace state. Assuming 'Default'."); - statusBar.setTarget("Default"); - // If no particular target is defined in settings, use 'Default' for the button - // but keep the variable empty, to not append it to the make command. - currentTarget = ""; + logger.message(`No target defined in the workspace state. Assuming '${currentTarget}'.`); } else { - currentTarget = buildTarget; logger.message(`Reading current build target "${currentTarget}" from the workspace state.`); - statusBar.setTarget(currentTarget); } + statusBar.setTarget(currentTarget); } let configureOnOpen: boolean | undefined; diff --git a/src/make.ts b/src/make.ts index 21e5a403..7bccb43c 100644 --- a/src/make.ts +++ b/src/make.ts @@ -446,17 +446,6 @@ export async function generateParseContent(progress: vscode.Progress<{}>, // Continue with the make dryrun invocation let makeArgs: string[] = []; - // Prepend the target to the arguments given in the makefile.configurations object, - // unless we want to parse for the full set of available targets. - if (forTargets) { - makeArgs.push("all"); - } else { - let currentTarget: string | undefined = configuration.getCurrentTarget(); - if (currentTarget) { - makeArgs.push(currentTarget); - } - } - // Include all the make arguments defined in makefile.configurations.makeArgs makeArgs = makeArgs.concat(configuration.getConfigurationMakeArgs()); @@ -492,6 +481,9 @@ export async function generateParseContent(progress: vscode.Progress<{}>, } }); + // Append the target to the arguments given in the makefile.configurations object + makeArgs.push(configuration.getCurrentTarget()); + logger.messageNoCR(`Generating ${getConfigureIsInBackground() ? "in the background a new " : ""}configuration cache with command: `); } diff --git a/src/test/fakeSuite/extension.test.ts b/src/test/fakeSuite/extension.test.ts index 10e7f8bb..b0bf089c 100644 --- a/src/test/fakeSuite/extension.test.ts +++ b/src/test/fakeSuite/extension.test.ts @@ -64,7 +64,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); @@ -118,7 +117,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); @@ -173,7 +171,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); @@ -250,7 +247,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); @@ -312,7 +308,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); @@ -374,7 +369,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); @@ -434,7 +428,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); @@ -476,7 +469,6 @@ suite('Fake dryrun parsing', /*async*/() => { await vscode.workspace.getConfiguration("makefile").update("launchConfigurations", undefined); configuration.setCurrentLaunchConfiguration(undefined); configuration.setCurrentMakefileConfiguration("Default"); - configuration.setCurrentTarget(undefined); configuration.initFromState(); await configuration.initFromSettings(); From ba6955518d39370a23d4af5293e33ae909d612e4 Mon Sep 17 00:00:00 2001 From: Radu Hociung Date: Fri, 13 Oct 2023 01:33:03 -0400 Subject: [PATCH 2/2] Avoid infinite dry-run loop with automake projects 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. --- package.nls.json | 2 +- src/configuration.ts | 30 +++++++++++++----------------- src/make.ts | 4 ++++ 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/package.nls.json b/package.nls.json index 70aa16cf..ab07361d 100644 --- a/package.nls.json +++ b/package.nls.json @@ -23,7 +23,7 @@ "makefile-tools.configuration.makefile.makePath.description": "The path to the make tool", "makefile-tools.configuration.makefile.configurations.description": "The user defined makefile configurations", "makefile-tools.configuration.makefile.configurations.name.description": "The name of the makefile configuration", - "makefile-tools.configuration.makefile.configurations.makefilePath.description": "File path to the makefile", + "makefile-tools.configuration.makefile.configurations.makefilePath.description": "File path to the makefile. Defaults to 'Makefile'", "makefile-tools.configuration.makefile.configurations.makePath.description": "File path to the make command", "makefile-tools.configuration.makefile.configurations.makeDirectory.description": "Folder path passed to make via the -C switch", "makefile-tools.configuration.makefile.configurations.makeArgs.description": "Arguments to pass to the make command", diff --git a/src/configuration.ts b/src/configuration.ts index ae2208b4..0f93e1b8 100644 --- a/src/configuration.ts +++ b/src/configuration.ts @@ -194,35 +194,37 @@ async function readMakePath(): Promise { } } -let makefilePath: string | undefined; -export function getMakefilePath(): string | undefined { return makefilePath; } -export function setMakefilePath(path: string): void { makefilePath = path; } +let makefilePath: string = "Makefile"; +export function getMakefilePath(): string { return makefilePath; } +export function setMakefilePath(path: string = "Makefile"): void { makefilePath = path; } // Read the full path to the makefile if defined in settings. // It represents a default to look for if no other makefile is already provided // in makefile.configurations.makefilePath. // TODO: validate and integrate with "-f [Makefile]" passed in makefile.configurations.makeArgs. async function readMakefilePath(): Promise { - makefilePath = await util.getExpandedSetting("makefilePath"); - if (!makefilePath) { + let makefilePathSetting: string | undefined = await util.getExpandedSetting("makefilePath"); + if (!makefilePathSetting) { logger.message("No path to the makefile is defined in the settings file."); + setMakefilePath(); } else { - makefilePath = util.resolvePathToRoot(makefilePath); + setMakefilePath(makefilePathSetting); } } let makeDirectory: string | undefined; export function getMakeDirectory(): string | undefined { return makeDirectory; } -export function setMakeDirectory(dir: string): void { makeDirectory = dir; } +export function setMakeDirectory(dir: string = ''): void { makeDirectory = dir; } // Read the make working directory path if defined in settings. // It represents a default to look for if no other makeDirectory is already provided // in makefile.configurations.makeDirectory. // TODO: validate and integrate with "-C [DIR_PATH]" passed in makefile.configurations.makeArgs. async function readMakeDirectory(): Promise { - makeDirectory = await util.getExpandedSetting("makeDirectory"); - if (!makeDirectory) { + let makeDirectorySetting: string | undefined = await util.getExpandedSetting("makeDirectory"); + if (!makeDirectorySetting) { logger.message("No folder path to the makefile is defined in the settings file."); + setMakeDirectory(); } else { - makeDirectory = util.resolvePathToRoot(makeDirectory); + setMakeDirectory(makeDirectorySetting); } } @@ -806,7 +808,7 @@ export async function getCommandForConfiguration(configuration: string | undefin } } - if (!util.checkFileExistsSync(configurationMakefile)) { + if (!util.checkFileExistsSync(util.resolvePathToRoot(configurationMakefile))) { logger.message("The makefile entry point was not found. " + "Make sure it exists at the location defined by makefile.makefilePath, makefile.configurations[].makefilePath, " + "makefile.makeDirectory, makefile.configurations[].makeDirectory" + @@ -1305,9 +1307,6 @@ export async function initFromSettings(activation: boolean = false): Promise(subKey); - if (updatedMakefilePath) { - updatedMakefilePath = util.resolvePathToRoot(updatedMakefilePath); - } if (updatedMakefilePath !== makefilePath) { // A change in makefile.makefilePath should trigger an IntelliSense update // only if the extension is not currently reading from a build log. @@ -1319,9 +1318,6 @@ export async function initFromSettings(activation: boolean = false): Promise(subKey); - if (updatedMakeDirectory) { - updatedMakeDirectory = util.resolvePathToRoot(updatedMakeDirectory); - } if (updatedMakeDirectory !== makeDirectory) { // A change in makefile.makeDirectory should trigger an IntelliSense update // only if the extension is not currently reading from a build log. diff --git a/src/make.ts b/src/make.ts index 7bccb43c..93348320 100644 --- a/src/make.ts +++ b/src/make.ts @@ -467,7 +467,11 @@ export async function generateParseContent(progress: vscode.Progress<{}>, makeArgs.push("--question"); logger.messageNoCR("Generating targets information with command: "); } else { + const makefilePath :string = configuration.getMakefilePath(); makeArgs.push("--dry-run"); + makeArgs.push(`--assume-old=${makefilePath}`); + makeArgs.push(makefilePath); + makeArgs.push("AM_MAKEFLAGS=--assume-old=Makefile Makefile"); // If this is not a clean configure, remove --always-make from the arguments list. // We need to have --always-make in makefile.dryrunSwitches and remove it for not clean configure