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] Add support for common/temp/.rush-override #5000

Closed
wants to merge 2 commits into from

Conversation

witcher112
Copy link
Contributor

Summary

While contributing to Rush, I've discovered that there's no easy way to test it with other repos on my PC (I was manually replacing installation in AppData/.rush directory) so I've decided to create this PR that hopefully makes it easier.

Details

PR introduces support for common/temp/.rush-override file that can contain path to the custom rush installation. This way, contributors can override the Rush installation used by the repository and test any changes that are being made to Rush.

.rush-override shouldn't be ever tracked by git so I've decided to use common/temp directory as it should be added to .gitignore in every Rush monorepo.

If common/temp/.rush-override is present, user is presented with warning similar to the one that is displayed when using RUSH_PREVIEW_VERSION environment variable.

image

(one potential thing I would like to include here is for a warning to display the path to custom Rush installation)

If common/temp/.rush-override points to invalid Rush installation (the only check for now is verifying that package.json exists), command terminates with following error:

image

How it was tested

I've created common/temp/.rush-override that points to custom Rush installation and executed node apps\rush\bin\rush build --help - seems to be working.

Impacted documentation

@witcher112
Copy link
Contributor Author

witcher112 commented Nov 9, 2024

I've just learned about https://rushjs.io/pages/contributing/#testing-rush-builds (right on time haha!)

But still, IMHO .rush-override could make it much easier (for example I tend to put a lot of commands in VSCode tasks.json, testing them against custom rush installation would require replacing all occurrences of rush and rushx).

@iclanton
Copy link
Member

@dmichon-msft / @D4N14L have a potential concern about dropping this file in common/temp.

@witcher112
Copy link
Contributor Author

After using it for a while, I do too - turns out rush purge manages to delete it (and I think it shouldn't).

Overall, I don't think it's going to be widely used that much (most probably only by contributors) to put it in already .gitignored place.

So perhaps storing it right next to rush.json should be fine, what do you think?

@iclanton
Copy link
Member

Yeah I think putting it at the repo root is a better approach.

Copy link
Member

@iclanton iclanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just using an environment variable to override the rush installation/@microsoft/rush-lib package location?

const overrideVersion: string = overridePackageJson.version;

const lines: string[] = [];
lines.push(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a printInBox utility function in @rushstack/terminal.

path.dirname(configuration.rushJsonFilename),
rushLib.RushConstants.commonFolderName,
rushLib.RushConstants.rushTempFolderName,
'.rush-override'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stick this in a constant.


if (configuration) {
const rushOverrideFilePath: string = path.join(
path.dirname(configuration.rushJsonFilename),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path.dirname(configuration.rushJsonFilename),
configuration.rushJsonFolder,

Comment on lines +52 to +57
const rushOverrideFilePath: string = path.join(
path.dirname(configuration.rushJsonFilename),
rushLib.RushConstants.commonFolderName,
rushLib.RushConstants.rushTempFolderName,
'.rush-override'
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const rushOverrideFilePath: string = path.join(
path.dirname(configuration.rushJsonFilename),
rushLib.RushConstants.commonFolderName,
rushLib.RushConstants.rushTempFolderName,
'.rush-override'
);
const rushOverrideFilePath: string = `${configuration.rushJsonFolder}/.rush-override`;

As per discussion.

Also stick .rush-override in a RushConstants.

'.rush-override'
);

if (fs.existsSync(rushOverrideFilePath)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use FileSystem from node-core-library, and do a try-catch on reading the file and gracefully handle it not existing. See FileSystem.isNotExistError

);

if (fs.existsSync(rushOverrideFilePath)) {
const overridePath: string = fs.readFileSync(rushOverrideFilePath, 'utf8').trim();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use FileSystem.readFile

const overridePackageJson: IPackageJson | undefined = PackageJsonLookup.instance.tryLoadPackageJsonFor(overridePath);

if (overridePackageJson === undefined) {
console.log(Colorize.red(`Cannot use common/temp/.rush-override file as it doesn't point to valid Rush package`));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use constants here.

Comment on lines +57 to +59
public get rushJsonFilename(): string {
return this._rushJsonFilename;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.

@witcher112
Copy link
Contributor Author

@iclanton thanks for the review!

I've decided to pursue the idea of an environment variable (I don't know why I didn't in the first place... :P)

Here's a new PR that includes rework as well as changes requested by you: #5003

@witcher112 witcher112 closed this Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants