Skip to content
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

[rush] Fix an issue where "rush list --json" prints non-json output in a repo that uses rush plugins with autoinstallers. #3391

Merged
merged 3 commits into from
May 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/changes/@microsoft/rush/fix-json-flag_2022-05-03-21-15.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Fix an issue where \"rush list --json\" prints non-json output in a repo that uses rush plugins with autoinstallers.",
"type": "none"
}
],
"packageName": "@microsoft/rush"
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,3 @@ exports[`CommandLineConfiguration Forbids a misnamed phase 3`] = `"In command-li
exports[`CommandLineConfiguration Forbids a misnamed phase 4`] = `"In command-line.json, the phase \\"_phase:A\\"'s name is not a valid phase name. Phase names must begin with the required prefix \\"_phase:\\" followed by a name containing lowercase letters, numbers, or hyphens. The name must start with a letter and must not end with a hyphen."`;

exports[`CommandLineConfiguration Forbids a misnamed phase 5`] = `"In command-line.json, the phase \\"_phase:A-\\"'s name is not a valid phase name. Phase names must begin with the required prefix \\"_phase:\\" followed by a name containing lowercase letters, numbers, or hyphens. The name must start with a letter and must not end with a hyphen."`;

exports[`CommandLineConfiguration parameters does not allow a parameter to only be associated with phased commands but not have any associated phases 1`] = `"command-line.json defines a parameter \\"--flag\\" that is only associated with phased commands, but lists no associated phases."`;
6 changes: 4 additions & 2 deletions libraries/rush-lib/src/cli/RushCommandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export class RushCommandLineParser extends CommandLineParser {

private _debugParameter!: CommandLineFlagParameter;
private _quietParameter!: CommandLineFlagParameter;
private _restrictConsoleOutput: boolean = RushCommandLineParser.shouldRestrictConsoleOutput();
private readonly _rushOptions: IRushCommandLineParserOptions;
private readonly _terminalProvider: ConsoleTerminalProvider;
private readonly _terminal: Terminal;
Expand All @@ -98,7 +99,7 @@ export class RushCommandLineParser extends CommandLineParser {
try {
const rushJsonFilename: string | undefined = RushConfiguration.tryFindRushJsonLocation({
startingFolder: this._rushOptions.cwd,
showVerbose: !RushCommandLineParser.shouldRestrictConsoleOutput()
showVerbose: !this._restrictConsoleOutput
});
if (rushJsonFilename) {
this.rushConfiguration = RushConfiguration.loadFromConfigurationFile(rushJsonFilename);
Expand All @@ -121,7 +122,8 @@ export class RushCommandLineParser extends CommandLineParser {
rushSession: this.rushSession,
rushConfiguration: this.rushConfiguration,
terminal: this._terminal,
builtInPluginConfigurations: this._rushOptions.builtInPluginConfigurations
builtInPluginConfigurations: this._rushOptions.builtInPluginConfigurations,
restrictConsoleOutput: this._restrictConsoleOutput
});

this._populateActions();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ export class InitAutoinstallerAction extends BaseRushAction {
protected async runAsync(): Promise<void> {
const autoinstallerName: string = this._name.value!;

const autoinstaller: Autoinstaller = new Autoinstaller(autoinstallerName, this.rushConfiguration);
const autoinstaller: Autoinstaller = new Autoinstaller({
autoinstallerName,
rushConfiguration: this.rushConfiguration
});

if (FileSystem.exists(autoinstaller.folderFullPath)) {
// It's okay if the folder is empty
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export class UpdateAutoinstallerAction extends BaseRushAction {
public constructor(parser: RushCommandLineParser) {
super({
actionName: 'update-autoinstaller',
summary: 'Updates autoinstaller package dependenices',
summary: 'Updates autoinstaller package dependencies',
documentation: 'Use this command to regenerate the shrinkwrap file for an autoinstaller folder.',
parser
});
Expand All @@ -32,7 +32,10 @@ export class UpdateAutoinstallerAction extends BaseRushAction {
protected async runAsync(): Promise<void> {
const autoinstallerName: string = this._name.value!;

const autoinstaller: Autoinstaller = new Autoinstaller(autoinstallerName, this.rushConfiguration);
const autoinstaller: Autoinstaller = new Autoinstaller({
autoinstallerName,
rushConfiguration: this.rushConfiguration
});
autoinstaller.update();

console.log('\nSuccess.');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,10 @@ export class GlobalScriptAction extends BaseScriptAction<IGlobalCommandConfig> {
}

private async _prepareAutoinstallerName(): Promise<void> {
const autoInstaller: Autoinstaller = new Autoinstaller(this._autoinstallerName, this.rushConfiguration);
const autoInstaller: Autoinstaller = new Autoinstaller({
autoinstallerName: this._autoinstallerName,
rushConfiguration: this.rushConfiguration
});

await autoInstaller.prepareAsync();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Positional arguments:
repo, and create or update the shrinkwrap file as
needed
update-autoinstaller
Updates autoinstaller package dependenices
Updates autoinstaller package dependencies
update-cloud-credentials
(EXPERIMENTAL) Update the credentials used by the
build cache provider.
Expand Down
62 changes: 40 additions & 22 deletions libraries/rush-lib/src/logic/Autoinstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,24 @@ import { RushGlobalFolder } from '../api/RushGlobalFolder';
import { RushConstants } from './RushConstants';
import { LastInstallFlag } from '../api/LastInstallFlag';

interface IAutoinstallerOptions {
autoinstallerName: string;
rushConfiguration: RushConfiguration;
restrictConsoleOutput?: boolean;
}

export class Autoinstaller {
public name: string;
public readonly name: string;

private _rushConfiguration: RushConfiguration;
private readonly _rushConfiguration: RushConfiguration;
private readonly _restrictConsoleOutput: boolean;

public constructor(autoinstallerName: string, rushConfiguration: RushConfiguration) {
this._rushConfiguration = rushConfiguration;
Autoinstaller.validateName(autoinstallerName);
this.name = autoinstallerName;
public constructor(options: IAutoinstallerOptions) {
this.name = options.autoinstallerName;
this._rushConfiguration = options.rushConfiguration;
this._restrictConsoleOutput = options.restrictConsoleOutput || false;

Autoinstaller.validateName(this.name);
}

// Example: .../common/autoinstallers/my-task
Expand Down Expand Up @@ -68,7 +77,8 @@ export class Autoinstaller {
await InstallHelpers.ensureLocalPackageManager(
this._rushConfiguration,
rushGlobalFolder,
RushConstants.defaultMaxInstallAttempts
RushConstants.defaultMaxInstallAttempts,
this._restrictConsoleOutput
);

// Example: common/autoinstallers/my-task/package.json
Expand All @@ -77,7 +87,7 @@ export class Autoinstaller {
autoinstallerFullPath
);

console.log(`Acquiring lock for "${relativePathForLogs}" folder...`);
this._logIfConsoleOutputIsNotRestricted(`Acquiring lock for "${relativePathForLogs}" folder...`);

const lock: LockFile = await LockFile.acquire(autoinstallerFullPath, 'autoinstaller');

Expand All @@ -104,14 +114,14 @@ export class Autoinstaller {
const nodeModulesFolder: string = path.join(autoinstallerFullPath, 'node_modules');

if (FileSystem.exists(nodeModulesFolder)) {
console.log('Deleting old files from ' + nodeModulesFolder);
this._logIfConsoleOutputIsNotRestricted('Deleting old files from ' + nodeModulesFolder);
FileSystem.ensureEmptyFolder(nodeModulesFolder);
}

// Copy: .../common/autoinstallers/my-task/.npmrc
Utilities.syncNpmrc(this._rushConfiguration.commonRushConfigFolder, autoinstallerFullPath);

console.log(`Installing dependencies under ${autoinstallerFullPath}...\n`);
this._logIfConsoleOutputIsNotRestricted(`Installing dependencies under ${autoinstallerFullPath}...\n`);

Utilities.executeCommand({
command: this._rushConfiguration.packageManagerToolFilename,
Expand All @@ -123,9 +133,9 @@ export class Autoinstaller {
// Create file: ../common/autoinstallers/my-task/.rush/temp/last-install.flag
lastInstallFlag.create();

console.log('Auto install completed successfully\n');
this._logIfConsoleOutputIsNotRestricted('Auto install completed successfully\n');
} else {
console.log('Autoinstaller folder is already up to date\n');
this._logIfConsoleOutputIsNotRestricted('Autoinstaller folder is already up to date\n');
}

lock.release();
Expand All @@ -138,13 +148,15 @@ export class Autoinstaller {
throw new Error(`The specified autoinstaller path does not exist: ` + autoinstallerPackageJsonPath);
}

console.log(`Updating autoinstaller package: ${autoinstallerPackageJsonPath}`);
this._logIfConsoleOutputIsNotRestricted(
`Updating autoinstaller package: ${autoinstallerPackageJsonPath}`
);

let oldFileContents: string = '';

if (FileSystem.exists(this.shrinkwrapFilePath)) {
oldFileContents = FileSystem.readFile(this.shrinkwrapFilePath, { convertLineEndings: NewlineKind.Lf });
console.log('Deleting ' + this.shrinkwrapFilePath);
this._logIfConsoleOutputIsNotRestricted('Deleting ' + this.shrinkwrapFilePath);
FileSystem.deleteFile(this.shrinkwrapFilePath);
}

Expand All @@ -158,7 +170,7 @@ export class Autoinstaller {
);
}

console.log();
this._logIfConsoleOutputIsNotRestricted();

Utilities.syncNpmrc(this._rushConfiguration.commonRushConfigFolder, this.folderFullPath);

Expand All @@ -169,18 +181,18 @@ export class Autoinstaller {
keepEnvironment: true
});

console.log();
this._logIfConsoleOutputIsNotRestricted();

if (this._rushConfiguration.packageManager === 'npm') {
console.log(colors.bold('Running "npm shrinkwrap"...'));
this._logIfConsoleOutputIsNotRestricted(colors.bold('Running "npm shrinkwrap"...'));
Utilities.executeCommand({
command: this._rushConfiguration.packageManagerToolFilename,
args: ['shrinkwrap'],
workingDirectory: this.folderFullPath,
keepEnvironment: true
});
console.log('"npm shrinkwrap" completed');
console.log();
this._logIfConsoleOutputIsNotRestricted('"npm shrinkwrap" completed');
this._logIfConsoleOutputIsNotRestricted();
}

if (!FileSystem.exists(this.shrinkwrapFilePath)) {
Expand All @@ -193,12 +205,18 @@ export class Autoinstaller {
convertLineEndings: NewlineKind.Lf
});
if (oldFileContents !== newFileContents) {
console.log(
this._logIfConsoleOutputIsNotRestricted(
colors.green('The shrinkwrap file has been updated.') + ' Please commit the updated file:'
);
console.log(`\n ${this.shrinkwrapFilePath}`);
this._logIfConsoleOutputIsNotRestricted(`\n ${this.shrinkwrapFilePath}`);
} else {
console.log(colors.green('Already up to date.'));
this._logIfConsoleOutputIsNotRestricted(colors.green('Already up to date.'));
}
}

private _logIfConsoleOutputIsNotRestricted(message?: string): void {
if (!this._restrictConsoleOutput) {
console.log(message);
}
}
}
36 changes: 27 additions & 9 deletions libraries/rush-lib/src/logic/installManager/InstallHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,25 @@ export class InstallHelpers {
public static async ensureLocalPackageManager(
rushConfiguration: RushConfiguration,
rushGlobalFolder: RushGlobalFolder,
maxInstallAttempts: number
maxInstallAttempts: number,
restrictConsoleOutput?: boolean
): Promise<void> {
let logIfConsoleOutputIsNotRestricted: (message?: string) => void;
if (restrictConsoleOutput) {
logIfConsoleOutputIsNotRestricted = () => {
/* noop */
};
} else {
logIfConsoleOutputIsNotRestricted = (message?: string) => {
console.log(message);
};
}

// Example: "C:\Users\YourName\.rush"
const rushUserFolder: string = rushGlobalFolder.nodeSpecificPath;

if (!FileSystem.exists(rushUserFolder)) {
console.log('Creating ' + rushUserFolder);
logIfConsoleOutputIsNotRestricted('Creating ' + rushUserFolder);
FileSystem.ensureFolder(rushUserFolder);
}

Expand All @@ -95,14 +107,16 @@ export class InstallHelpers {
node: process.versions.node
});

console.log(`Trying to acquire lock for ${packageManagerAndVersion}`);
logIfConsoleOutputIsNotRestricted(`Trying to acquire lock for ${packageManagerAndVersion}`);

const lock: LockFile = await LockFile.acquire(rushUserFolder, packageManagerAndVersion);

console.log(`Acquired lock for ${packageManagerAndVersion}`);
logIfConsoleOutputIsNotRestricted(`Acquired lock for ${packageManagerAndVersion}`);

if (!packageManagerMarker.isValid() || lock.dirtyWhenAcquired) {
console.log(colors.bold(`Installing ${packageManager} version ${packageManagerVersion}${os.EOL}`));
logIfConsoleOutputIsNotRestricted(
colors.bold(`Installing ${packageManager} version ${packageManagerVersion}${os.EOL}`)
);

// note that this will remove the last-install flag from the directory
Utilities.installPackageInDirectory({
Expand All @@ -120,9 +134,13 @@ export class InstallHelpers {
commonRushConfigFolder: rushConfiguration.commonRushConfigFolder
});

console.log(`Successfully installed ${packageManager} version ${packageManagerVersion}`);
logIfConsoleOutputIsNotRestricted(
`Successfully installed ${packageManager} version ${packageManagerVersion}`
);
} else {
console.log(`Found ${packageManager} version ${packageManagerVersion} in ${packageManagerToolFolder}`);
logIfConsoleOutputIsNotRestricted(
`Found ${packageManager} version ${packageManagerVersion} in ${packageManagerToolFolder}`
);
}

packageManagerMarker.create();
Expand All @@ -136,8 +154,8 @@ export class InstallHelpers {
`${packageManager}-local`
);

console.log(os.EOL + `Symlinking "${localPackageManagerToolFolder}"`);
console.log(` --> "${packageManagerToolFolder}"`);
logIfConsoleOutputIsNotRestricted(os.EOL + `Symlinking "${localPackageManagerToolFolder}"`);
logIfConsoleOutputIsNotRestricted(` --> "${packageManagerToolFolder}"`);

// We cannot use FileSystem.exists() to test the existence of a symlink, because it will
// return false for broken symlinks. There is no way to test without catching an exception.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ import {
PluginLoaderBase
} from './PluginLoaderBase';

interface IAutoinstallerPluginLoaderOptions extends IPluginLoaderOptions<IRushPluginConfiguration> {
restrictConsoleOutput: boolean;
}

/**
* @beta
*/
Expand All @@ -22,12 +26,13 @@ export class AutoinstallerPluginLoader extends PluginLoaderBase<IRushPluginConfi

public readonly packageFolder: string;

public constructor(options: IPluginLoaderOptions<IRushPluginConfiguration>) {
public constructor(options: IAutoinstallerPluginLoaderOptions) {
super(options);
this._autoinstaller = new Autoinstaller(
options.pluginConfiguration.autoinstallerName,
this._rushConfiguration
);
this._autoinstaller = new Autoinstaller({
autoinstallerName: options.pluginConfiguration.autoinstallerName,
rushConfiguration: this._rushConfiguration,
restrictConsoleOutput: options.restrictConsoleOutput
});

this.packageFolder = path.join(this._autoinstaller.folderFullPath, 'node_modules', this.packageName);
}
Expand Down
6 changes: 5 additions & 1 deletion libraries/rush-lib/src/pluginFramework/PluginManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface IPluginManagerOptions {
rushConfiguration: RushConfiguration;
rushSession: RushSession;
builtInPluginConfigurations: IBuiltInPluginConfiguration[];
restrictConsoleOutput: boolean;
}

export interface ICustomCommandLineConfigurationInfo {
Expand All @@ -27,6 +28,7 @@ export class PluginManager {
private readonly _terminal: ITerminal;
private readonly _rushConfiguration: RushConfiguration;
private readonly _rushSession: RushSession;
private readonly _restrictConsoleOutput: boolean;
private readonly _builtInPluginLoaders: BuiltInPluginLoader[];
private readonly _autoinstallerPluginLoaders: AutoinstallerPluginLoader[];
private readonly _installedAutoinstallerNames: Set<string>;
Expand All @@ -38,6 +40,7 @@ export class PluginManager {
this._terminal = options.terminal;
this._rushConfiguration = options.rushConfiguration;
this._rushSession = options.rushSession;
this._restrictConsoleOutput = options.restrictConsoleOutput;

this._installedAutoinstallerNames = new Set<string>();

Expand Down Expand Up @@ -93,7 +96,8 @@ export class PluginManager {
return new AutoinstallerPluginLoader({
pluginConfiguration,
rushConfiguration: this._rushConfiguration,
terminal: this._terminal
terminal: this._terminal,
restrictConsoleOutput: this._restrictConsoleOutput
});
});
}
Expand Down