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.

Sign up for GitHub

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

Jump to bottom

Removing more unwraps from FFI layer #18370

Merged
merged 9 commits into from
Mar 7, 2023
Merged

Removing more unwraps from FFI layer #18370

merged 9 commits into from
Mar 7, 2023

Conversation

seve-martinez
Copy link
Contributor

Clears out more of #13608.

Key changes:

  • Removed unwraps from FFI layer, mainly in nodes.rs.
  • Removed redundant clone().try_into().unwrap() that converted a Process to a Process in the remote_tests.rs. This seems to have been legacy from when Process was actually MEPR.
  • Renamed input_files to inputs in the InputDigests struct, inline with the immutable_inputs attribute.

@seve-martinez seve-martinez force-pushed the smartinez/removing_more_unwraps_from_ffi branch from 7ead024 to a35eff1 Compare February 26, 2023 20:05
@seve-martinez seve-martinez marked this pull request as draft February 26, 2023 20:06
@seve-martinez seve-martinez marked this pull request as ready for review February 26, 2023 20:21
@stuhood stuhood added the category:internal CI, fixes for not-yet-released features, etc. label Mar 3, 2023
stuhood
stuhood approved these changes Mar 3, 2023
View reviewed changes
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

src/rust/engine/process_execution/src/lib.rs Show resolved Hide resolved
src/rust/engine/src/context.rs Outdated Show resolved Hide resolved
src/rust/engine/src/nodes.rs Outdated Show resolved Hide resolved
src/rust/engine/src/nodes.rs Outdated Show resolved Hide resolved
seve-martinez and others added 2 commits March 3, 2023 13:08
@seve-martinez @stuhood
Update src/rust/engine/src/context.rs
481befd 
Co-authored-by: Stu Hood <stuhood@gmail.com>
@seve-martinez @stuhood
Update src/rust/engine/src/nodes.rs
8601f8e 
Co-authored-by: Stu Hood <stuhood@gmail.com>
@benjyw
Copy link
Contributor

benjyw commented Mar 7, 2023

@stuhood is this ready to merge, and if so can you merge it?

@seve-martinez
Copy link
Contributor Author

@stuhood is this ready to merge, and if so can you merge it?

@benjyw sorry for the delay, this should be good to go now!

@seve-martinez seve-martinez reopened this Mar 7, 2023
stuhood
stuhood approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

@stuhood stuhood enabled auto-merge (squash) March 7, 2023 17:47
@stuhood stuhood merged commit d88e258 into pantsbuild:main Mar 7, 2023
@Eric-Arellano Eric-Arellano mentioned this pull request Mar 11, 2023
Merged
Eric-Arellano
Eric-Arellano reviewed Mar 11, 2023
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This is great! Thanks for working on this!

@huonw huonw mentioned this pull request Sep 1, 2023
Open
@stuhood stuhood mentioned this pull request Dec 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eric-Arellano Eric-Arellano Eric-Arellano left review comments

@stuhood stuhood stuhood approved these changes

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seve-martinez @benjyw @stuhood @Eric-Arellano