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

rsc: Implement several bug fixes #1543

Merged
merged 2 commits into from
Apr 18, 2024
Merged

rsc: Implement several bug fixes #1543

merged 2 commits into from
Apr 18, 2024

Conversation

V-FEXrt
Copy link
Contributor

@V-FEXrt V-FEXrt commented Apr 16, 2024

The following bugs are fixed in this PR

  • Logging is temporarily disabled due to spammy logs creating a 2x slowdown
  • Output files were previously being written to the wrong location when the Plan directory was changed
  • Symlink content is now entirely defined by the content of the link, not a vague notion of the source path
  • Symlinks would fail if a symlink was left on disk from a previous build
  • The source path of the symlink was being returned as the output instead of the symlink itself
  • Symlinks no longer wait for all files to download as it's not necessary

With these changes larger internal builds will complete successfully

Copy link
Collaborator

@ag-eitilt ag-eitilt left a comment

Choose a reason for hiding this comment

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

Seems good so far as I can tell.

Comment on lines +336 to +337
// TODO: The logging is incredibly spammy right now and causes significant slow down.
// for now, logging is disabled but this should be turned back on once logging is pruned.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably just make this an option right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, though I want to spend some time to get it right. i.e. a matrix of where to log X what level to log at. I feel like it'll be totally fine for "log errors to file" as the default. I just wanted to focus on making it work first before circling back around to that.

@@ -162,66 +162,79 @@ export def mkRemoteCacheRunner (rscApi: RemoteCacheApi) (hashFn: Result RunnerIn
else failWithError "rsc: Failed to make output directory"

def doDownload (CacheSearchOutputFile path mode blob) =
rscApiGetFileBlob blob "{input.getRunnerInputDirectory}/{path}" mode
rscApiGetFileBlob blob path mode
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to write down in the RunnerInput/Plan tuple documentation that only Command is relative to Directory both Visible/Input and Output are relative to workspace. It seems reasonable to assume either and I don't see it written down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a comment/note on this line but it would probably be good for us to put together a high level document covering all these details/designs/limitations.

We have a bunch of smaller docs, but I don't think we have the top level doc covering all the content. Mainly because most of this was just iterating on the local cache learnings/design

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

LGTM. Happy to defer logging config until we have a whole solution.

@V-FEXrt V-FEXrt merged commit 25b1e48 into master Apr 18, 2024
12 checks passed
@V-FEXrt V-FEXrt deleted the wake-rsc-client-bugfixes branch April 18, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants