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

Drop actual_path? #33

Open
Arcterus opened this issue Jul 18, 2018 · 5 comments
Open

Drop actual_path? #33

Arcterus opened this issue Jul 18, 2018 · 5 comments

Comments

@Arcterus
Copy link
Contributor

I am considering removing util::actual_path() and just making all the utilities assume that the current working directory that we should operate in is the current working directory of the binary. The original point of util::actual_path() was to support the testing framework, but we won't need it for that once #24 is merged.

I can see it still being useful in some cases where mesabox is used as a library within another program as it allows operating in different directories without modifying the arguments or changing the host program's current working directory (which could be problematic if that program were to be multi-threaded). At the same time, removing util::actual_path() makes the utilities simpler and reduces the number of allocations we need to perform.

@Arcterus
Copy link
Contributor Author

If no one has a preference/feels like it is useful, I am leaning towards dropping it as I'm not sure the added complexity is worth it.

@Arcterus Arcterus reopened this Jul 18, 2018
@Arcterus
Copy link
Contributor Author

Hit the wrong button.

@Arcterus
Copy link
Contributor Author

One option might be to keep the file descriptor in UtilData rather than the name of the current directory and then use the *at functions (like openat) to access files (although we might need to keep the directory path as well to pass to crates we use). When changing directories we'd just have to replace the file descriptor (and path if we need to keep that as well).

This solution would help avoid the allocation issue while still maintaining the current functionality. The downsides are that writing utilities remains less ergonomic than normal binaries and that we'd need to either use nix/libc functions directory or use less vetted libraries (e.g. openat).

@mssun
Copy link
Contributor

mssun commented Jul 19, 2018

No preference for me. I think we can drop it to keep the the code clean and simple.

@Arcterus
Copy link
Contributor Author

Arcterus commented Aug 2, 2018

Only thing about this that needs consideration really is what to do with cd in sh. Currently, we try to avoid forking as much as possible, so we don't actually spawn real subshells if we can avoid doing so. The problem with that is that if the user changes the directory in one of the fake "subshells," the directory will changed outside of the "subshell."

My line of thought right now is that we should avoid forking until we see cd, in which case we fork and execute the rest of the commands for the current subshell in a real subshell. Doing so requires keeping track of whether we are in a fake "subshell" and ensuring we only fork when we haven't already done so for the current "subshell."

Another solution might be to change the directory and then change it back once the "subshell" exits, but this could cause issues if env::set_current_dir() fails after the current working directory has already been changed. This solution might also cause problems if we ever add multi-threading or other related features.

(Note that currently cd just doesn't worry about any of this and changes the directory no matter if it's in a subshell or not and it ignores actual_path(), so we will need to change it regardless of our solution).

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

No branches or pull requests

2 participants