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

Allow builtins to be executed as part of pipelines #469

Merged
merged 15 commits into from Jul 27, 2017
Merged

Allow builtins to be executed as part of pipelines #469

merged 15 commits into from Jul 27, 2017

Conversation

hgoldstein
Copy link
Contributor

@hgoldstein hgoldstein commented Jul 26, 2017

Problem: Currently if we have a builtin to be run as part of a pipeline, we just take the job, reconstruct it, and run a sub-process such as ion -c builtin arg1 arg2. This is problematic as it forces ion to re-parse the given arguments which is inefficient and bug prone ( #454 ).

Solution: Manually manage running builtins as part of a pipeline by forking the current process and executing the builtin in the child process.

Changes introduced by this pull request:

  • Replaced the use of Command in shell::pipe with RefinedJob; an enumeration that is either an external command or a builtin
  • Modified shell::pipe::pipe to fork and execute a builtin for the RefinedJob::Builtin instance; behavior for RefinedJob::External is roughly the same as before: build a Command, run spawn().

Drawbacks: The use of RawFd everywhere might make #364 more difficult.

Fixes: Closes #379 ; Closes #454

State: WIP; I want to make sure there are some regression tests in examples/. Also this is a 500+- change so letting it sit for review would be good.

TODO:

  • Determine the redox equivalent for STDOUT_FILENO et al
  • Add a regression example in examples/ for piping to / from a builtin #

@hgoldstein
Copy link
Contributor Author

Also it seems I forgot to add STDOUT_FILENO for redox, so definitely WIP.

@mmstick
Copy link
Contributor

mmstick commented Jul 26, 2017

Looks good to me so far.

@hgoldstein
Copy link
Contributor Author

hgoldstein commented Jul 26, 2017

Good to go; I wanted to add an example for read, but it doesn't work when stdin is not a tty.

EDIT: See #474

@mmstick
Copy link
Contributor

mmstick commented Jul 27, 2017

Thanks for this work!

@mmstick mmstick merged commit 83ea337 into redox-os:master Jul 27, 2017
@hgoldstein hgoldstein deleted the pipeline_builtins branch July 27, 2017 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants