Skip to content

[rush] Introduce dependsOnAdditionalFiles configuration option to operations in rush-project.json#3824

Merged
octogonz merged 6 commits into
microsoft:mainfrom
lukaskl:lukaskl/additional-files
Jan 20, 2023
Merged

[rush] Introduce dependsOnAdditionalFiles configuration option to operations in rush-project.json#3824
octogonz merged 6 commits into
microsoft:mainfrom
lukaskl:lukaskl/additional-files

Conversation

@lukaskl
Copy link
Copy Markdown
Contributor

@lukaskl lukaskl commented Dec 8, 2022

Summary

This PR allows repository maintainers to pass glob (minimatch) patters as dependsOnAdditionalFiles option to operations in rush-project.json.

If dependsOnAdditionalFiles is passed, rush will read matched files before said operation, hash their contents and include those hashes when calculating final hash for the build cache.

Fixes: #3822

Details

This PR is heavily based on #3769
I've noticed that solution used in #3769 is nearly identical to what I need, with just a few changes.

In addition to that - it keeps things nicely colocated and aligned.

  • Backwards compatibility shouldn't be broken as dependsOnAdditionalFiles is optional property
  • Impact on performance will depend heavily on how repository maintainers will use this feature. However, by default, if dependsOnAdditionalFiles is not provided (which I believe will be true for most of the repositories) the impact on performance should be nearly zero.

How it was tested

I've followed the guide described here: https://rushjs.io/pages/contributing/
I've set up alias for the ./rushstack/libraries/rush-lib/lib/start.js script and tried to build my own rush projects.

Impacted documentation

Comment thread libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts
Comment thread libraries/rush-lib/src/logic/buildCache/AdditionalFilesHasher.ts Outdated
Comment thread libraries/rush-lib/src/logic/buildCache/AdditionalFilesHasher.ts Outdated
@lukaskl
Copy link
Copy Markdown
Contributor Author

lukaskl commented Dec 8, 2022

@microsoft-github-policy-service agree

Comment thread libraries/rush-lib/src/logic/buildCache/AdditionalFilesHasher.ts Outdated
Comment thread libraries/rush-lib/src/logic/buildCache/AdditionalFilesHasher.ts Outdated
Comment thread libraries/rush-lib/src/logic/buildCache/AdditionalFilesHasher.ts Outdated
Use helper class provided by @rushstack/node-core-library instead of plain 'fs' package or manually converting callback to promise.
@lukaskl lukaskl force-pushed the lukaskl/additional-files branch from 6bef342 to 4f9fa41 Compare December 8, 2022 10:33
Comment thread libraries/rush-lib/src/logic/buildCache/AdditionalFilesHasher.ts Outdated
Comment thread libraries/rush-lib/src/logic/buildCache/AdditionalFilesHasher.ts Outdated
@dmichon-msft
Copy link
Copy Markdown
Contributor

An aside, I do wonder if it would be worth separating out two different config options for including files that are part of the repository but outside of the project working directory (e.g. that live in common/), vs. files that live outside of the repository (or are not tracked files). The reason being that for tracked files that merely live outside of the project folder, we can just fetch that from our existing map of hashes.

@lukaskl lukaskl force-pushed the lukaskl/additional-files branch from a2c56ed to c2fc78c Compare January 11, 2023 12:30
@lukaskl
Copy link
Copy Markdown
Contributor Author

lukaskl commented Jan 11, 2023

An aside, I do wonder if it would be worth separating out two different config options for including files that are part of the repository but outside of the project working directory (e.g. that live in common/), vs. files that live outside of the repository (or are not tracked files). The reason being that for tracked files that merely live outside of the project folder, we can just fetch that from our existing map of hashes.

I've added a new commit: 989175b

So first of all - I don't think there is a need to have two different config options, we simply can just check whether the path belongs to the repo and whether we have a hash for it already. In my opinion that is enough info to move further.

Furthermore, I've exposed repoState.hashes from the ProjectChangeAnalyzer and I've used this to reuse already known hashes. Not sure whether this is the nicest way to access this data. But unless I'm missing something - it seems that repo state data is embedded into ProjectChangeAnalyzer and extracting it would lead to a bigger refactoring, and I'm not sure it would be a great idea to do it with this PR 😅

@octogonz octogonz merged commit bbf3893 into microsoft:main Jan 20, 2023
octogonz added a commit that referenced this pull request Jan 21, 2023
@octogonz
Copy link
Copy Markdown
Collaborator

🚀 This was released with Rush 5.89.0.

@SSinkus
Copy link
Copy Markdown

SSinkus commented Mar 28, 2023

@octogonz since this PR is merged neither https://developer.microsoft.com/json-schemas/rush/v5/rush-project.schema.json or rushjs.io/pages/configs/rush-project_json is updated.

@lukaskl
Copy link
Copy Markdown
Contributor Author

lukaskl commented Mar 28, 2023

Just a heads up - if you'll want to use dependsOnAdditionalFiles or dependsOnEnvVars there are some misalignments with incremental builds rules, particular third one:

image

In essence, it seems that if dependsOnAdditionalFiles or dependsOnEnvVars makes one project to be rebuilt, it doesn't make all other remaining projects in the chain to be rebuilt.

We quickly solved this by duplicating dependsOnAdditionalFiles or dependsOnEnvVars in most of our projects and I still have a reminder to create a proper ticket on rushstack repository, but couldn't find time for it yet.

@SquGus
Copy link
Copy Markdown

SquGus commented Oct 10, 2023

@lukaskl did you ever create a proper ticket related to the misalignment with incremental builds? Asking because I am thinking of upgrading Rush to use the dependsOnAdditionalFiles option and what to understand if that ever got addressed.

@dmichon-msft
Copy link
Copy Markdown
Contributor

The work item to alter the cache key calculations to include the cache keys of the build tasks they depend on is still work to be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[rush] Allow to specify additional files (non tracked by git) for build cache

6 participants