Simplify process interface #3764

djc · 2024-04-09T13:55:23Z

This makes the Process abstraction much easier to understand (in my opinion) by avoiding a trait abstraction (which had only a single implementation) and explicitly relying on the enum variants. Removes 250 lines of code and the need for the enum-dispatch dependency.

rami3l · 2024-04-09T14:28:07Z

@djc Wow, thank you so much for the hard work! I will definitely find some time to review the patch.

OTOH I guess we can put this in 1.28? I really want to make 1.27.1 "patch only".

djc · 2024-04-09T15:24:16Z

I'm fine with holding this back until after 1.27.1 is released, though I also think the risk is pretty limited.

rbtcollins · 2024-04-09T17:38:27Z

I'm fine in principle with this. there is a similar structure in the fork of home that we use - https://github.com/rbtcollins/home/blob/master/src/lib.rs#L41 - so you may want to tackle that, or perhaps we pick up the discussion about having home natively support our needs again.

rbtcollins

One nit you can ignore, and a file rename request - we don't use the mod.rs style in rustup.

rbtcollins · 2024-04-09T17:28:27Z

src/cli/self_update.rs

@@ -1340,8 +1340,10 @@ mod tests {
                    .unwrap() // result
                    .unwrap() // option
                );
+


Not sure this vertical white space helps anything much :)

Personally I find the delineation of larger code blocks to help understand the structure of the code.

rbtcollins · 2024-04-09T17:58:18Z

src/process/mod.rs

@@ -0,0 +1,298 @@
+use std::cell::RefCell;


please call this file src/process.rs

rbtcollins · 2024-04-09T18:28:13Z

In terms of when to merge, this will conflict with #3367, which is already ready to merge but also waiting for 1.28 to open.

djc · 2024-04-09T19:28:51Z

I'm fine in principle with this. there is a similar structure in the fork of home that we use - https://github.com/rbtcollins/home/blob/master/src/lib.rs#L41 - so you may want to tackle that, or perhaps we pick up the discussion about having home natively support our needs again.

I think we're just using upstream version 0.5.4 right now?

One nit you can ignore, and a file rename request - we don't use the mod.rs style in rustup.

This is not actually true:

djc-2021 cfg-debug rustup $ find . -name "mod.rs"
./tests/suite/mod.rs
./download/tests/support/mod.rs
./src/diskio/mod.rs
./src/test/mock/mod.rs
./src/dist/component/mod.rs
./src/dist/mod.rs
./src/utils/mod.rs

Personally, I've grown to substantially dislike the style where the parent module root lives in a separate directory from the child modules, since it keeps them farther away in the directory view of IDEs and such, making the code harder to navigate at least for me. I've tried it on several projects after the 2018 edition went stable and have ended up mostly reverting to the earlier style -- and I checked to see that this style is already quite common in the rustup codebase.

In terms of when to merge, this will conflict with #3367, which is already ready to merge but also waiting for 1.28 to open.

Well, it has quite a bit of unaddressed review feedback -- but, I'm happy to let #3367 land before this PR.

rbtcollins · 2024-04-10T21:01:09Z

Oh, I missed all that feedaback, will look at it.

Personally, I've grown to substantially dislike the style where the parent module root lives in a separate directory from the child modules, since it keeps them farther away in the directory view of IDEs and such, making the code harder to navigate at least for me. I've tried it on several projects after the 2018 edition went stable and have ended up mostly reverting to the earlier style -- and I checked to see that this style is already quite common in the rustup codebase.

Ah, its hangovers we hadn't refactored. We've tended not to make nonfunctional changes for various not particularly good reasons.

Happy for us to have a discussion then about what style the project should take, but I've found the opposite - foo.rs means the module file path is always the same, whether or not the module has child modules, or is a leaf itself where a directory with just the module file would be tedious.

djc · 2024-04-11T07:22:05Z

Personally, I've grown to substantially dislike the style where the parent module root lives in a separate directory from the child modules, since it keeps them farther away in the directory view of IDEs and such, making the code harder to navigate at least for me. I've tried it on several projects after the 2018 edition went stable and have ended up mostly reverting to the earlier style -- and I checked to see that this style is already quite common in the rustup codebase.

Ah, its hangovers we hadn't refactored. We've tended not to make nonfunctional changes for various not particularly good reasons.

Happy for us to have a discussion then about what style the project should take, but I've found the opposite - foo.rs means the module file path is always the same, whether or not the module has child modules, or is a leaf itself where a directory with just the module file would be tedious.

Right. My preference would be to use single files when there are no child modules (in separate files), but I think as soon as we move to a directory it's better to keep everything contained in the same directory -- rather than having the code for a single module spread over two places, one file for the "root" and a directory containing further files for the children.

@rami3l any thoughts?

rami3l · 2024-04-11T08:35:56Z

Right. My preference would be to use single files when there are no child modules (in separate files), but I think as soon as we move to a directory it's better to keep everything contained in the same directory -- rather than having the code for a single module spread over two places, one file for the "root" and a directory containing further files for the children.

@rami3l any thoughts?

@djc @rbtcollins You're referring to foo/mod.rs vs foo.rs right? I use the latter in my projects, since I use path searching instead of path navigating (in Neovim, but previously in VSCode as well). When you use searching it's a nightmare to encounter a whole bunch of */mod.rss.

I do understand that this is a niche, and foo/mod.rs is much better if you're navigating thru the paths (probably has something to do with VSCode not wanting to put foo/ next to foo.rs in the explorer view). However, considering the compatibility for both cases I personally prefer foo.rs.

That being said, I'm eventually okay with either way as long as they are consistent (looks like that isn't the case currently), and BTW we should probably get rid of that dist::dist thing somewhere in the codebase which gets me every time...

djc · 2024-06-04T13:39:42Z

I've scaled this back a little for now -- it no longer moves the code around and just gets rid of the trait indirection in favor of an enum.

rami3l

The latest changes look great!

To be frank, this has really addressed one of my pain points when doing #3803. Thank you so much 🙏

PS: I guess we can have the foo.rs vs foo/mod.rs discussion somewhere else, possibly after merging this one.

src/currentprocess.rs

djc requested a review from rami3l April 9, 2024 13:55

djc force-pushed the process-tweaks branch from 9d3efcd to c70bcb6 Compare April 9, 2024 14:23

djc force-pushed the process-tweaks branch from c70bcb6 to 7e5da76 Compare April 9, 2024 15:23

djc force-pushed the process-tweaks branch 2 times, most recently from f452990 to 2e19e1f Compare April 9, 2024 16:05

rbtcollins reviewed Apr 9, 2024

View reviewed changes

rami3l added this to the 1.28.0 milestone Apr 13, 2024

djc added 8 commits June 4, 2024 15:38

Simplify process access to argument iterator

c528452

Simplify process access to stdout

5230106

Simplify process access to stderr

a8c07c2

Simplify process access to stdin

f3853f3

Simplify process access to pid

77acc98

Remove unnecessary trait bound for home::Env

3901c32

Simplify process access to environment variables

1885938

Simplify process access to current_dir

7260097

djc force-pushed the process-tweaks branch from 2e19e1f to 96c00e3 Compare June 4, 2024 13:38

Remove unnecessary trait abstraction

3e387fa

djc force-pushed the process-tweaks branch from 96c00e3 to 3e387fa Compare June 4, 2024 14:05

rami3l approved these changes Jun 4, 2024

View reviewed changes

djc enabled auto-merge June 4, 2024 14:13

rami3l reviewed Jun 4, 2024

View reviewed changes

src/currentprocess.rs Show resolved Hide resolved

djc added this pull request to the merge queue Jun 4, 2024

Merged via the queue into master with commit 59d15e6 Jun 4, 2024
24 checks passed

djc deleted the process-tweaks branch June 4, 2024 14:52

rami3l mentioned this pull request Jun 17, 2024

Style: Decide between foo.rs and foo/mod.rs #3883

Open

djc mentioned this pull request Jul 11, 2024

Fix home_dir() and current_dir() regression #3938

Merged

rami3l mentioned this pull request Oct 6, 2024

dist: bump rustup version to 1.28.0 #4041

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify process interface #3764

Simplify process interface #3764

djc commented Apr 9, 2024

rami3l commented Apr 9, 2024 •

edited

Loading

djc commented Apr 9, 2024

rbtcollins commented Apr 9, 2024

rbtcollins left a comment

rbtcollins Apr 9, 2024

djc Apr 9, 2024

rbtcollins Apr 9, 2024

rbtcollins commented Apr 9, 2024

djc commented Apr 9, 2024

rbtcollins commented Apr 10, 2024

djc commented Apr 11, 2024 •

edited

Loading

rami3l commented Apr 11, 2024 •

edited

Loading

djc commented Jun 4, 2024 •

edited

Loading

rami3l left a comment

Simplify process interface #3764

Simplify process interface #3764

Conversation

djc commented Apr 9, 2024

rami3l commented Apr 9, 2024 • edited Loading

djc commented Apr 9, 2024

rbtcollins commented Apr 9, 2024

rbtcollins left a comment

Choose a reason for hiding this comment

rbtcollins Apr 9, 2024

Choose a reason for hiding this comment

djc Apr 9, 2024

Choose a reason for hiding this comment

rbtcollins Apr 9, 2024

Choose a reason for hiding this comment

rbtcollins commented Apr 9, 2024

djc commented Apr 9, 2024

rbtcollins commented Apr 10, 2024

djc commented Apr 11, 2024 • edited Loading

rami3l commented Apr 11, 2024 • edited Loading

djc commented Jun 4, 2024 • edited Loading

rami3l left a comment

Choose a reason for hiding this comment

rami3l commented Apr 9, 2024 •

edited

Loading

djc commented Apr 11, 2024 •

edited

Loading

rami3l commented Apr 11, 2024 •

edited

Loading

djc commented Jun 4, 2024 •

edited

Loading