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

Split CLI into library and binary part #551

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Split CLI into library and binary part #551

wants to merge 3 commits into from

Conversation

phil-opp
Copy link
Collaborator

Enables other executables to invoke dora CLI commands directly without going through the argument parsing.

@haixuanTao
Copy link
Collaborator

To move forward with this PR, we need to remove std::env::current_exe(). :

Command::new(std::env::current_exe().wrap_err("failed to get current executable path")?);
from our cli as it will call the executable in which the cli library has been linked to

Enables other executables to invoke dora CLI commands directly without going through the argument parsing.
@phil-opp
Copy link
Collaborator Author

To move forward with this PR, we need to remove std::env::current_exe(). :

Command::new(std::env::current_exe().wrap_err("failed to get current executable path")?);

I replaced the current_exe calls with a dora_cli_path parameter in ce18beb. We still need a CLI executable that is capable of parsing the arguments and starting the coordinator, daemon, and runtime. So I'm not sure how useful this PR is in practice, since you still need to build some sort of dora binary even if you want to call into the dora library directly.

@haixuanTao
Copy link
Collaborator

To move forward with this PR, we need to remove std::env::current_exe(). :

Command::new(std::env::current_exe().wrap_err("failed to get current executable path")?);

I replaced the current_exe calls with a dora_cli_path parameter in ce18beb. We still need a CLI executable that is capable of parsing the arguments and starting the coordinator, daemon, and runtime. So I'm not sure how useful this PR is in practice, since you still need to build some sort of dora binary even if you want to call into the dora library directly.

To be honest, I fear that passing the pass to dora path might not necessarily be easy, might create confusion, and might make it hard for people to share code in machine-agnostic manner.

I was thinking about using which::which that would make the application behave with the same fashion of a terminal.

@haixuanTao
Copy link
Collaborator

To move forward with this PR, we need to remove std::env::current_exe(). :

Command::new(std::env::current_exe().wrap_err("failed to get current executable path")?);

I replaced the current_exe calls with a dora_cli_path parameter in ce18beb. We still need a CLI executable that is capable of parsing the arguments and starting the coordinator, daemon, and runtime. So I'm not sure how useful this PR is in practice, since you still need to build some sort of dora binary even if you want to call into the dora library directly.

I think that people like the idea of being able to spawn a dora application within python or Rust, so that they can wrap dora within their own application ( UI, library, ...)

I think that making it a library makes distributing dora application a lot easier.

@haixuanTao
Copy link
Collaborator

I have pushed a new branch with the changes to use which::which : https://github.com/dora-rs/dora/compare/split-cli...split-cli-with-which?expand=1

@phil-opp
Copy link
Collaborator Author

I'm not sure if which is good idea. There is no guarantee that dora is installed, that it is in the PATH, and that it's the same version as the binary.

Maybe it's possible to spawn the deamon/coordinator/runtime as threads instead of subprocesses? Or we could use fork on Unix systems. Or we could require some basic form of argument parsing when the dora library is used, then we could continue using current_exe.

@haixuanTao
Copy link
Collaborator

haixuanTao commented Jul 31, 2024

deamon/coordinator/runtime as threads.

That could be a good idea and it could scope the up environment to the duration of the initial process.

It seems that fork has some issue: https://internals.rust-lang.org/t/why-no-fork-in-std-process/13770

If at the end of the day. If this is too hard, let's just wrap start and stop and leave up for another PR.

@phil-opp
Copy link
Collaborator Author

Yeah, I'm not a fan of using fork either.

deamon/coordinator/runtime as threads.

That could be a good idea and it could scope the up environment to the duration of the initial process.

Ok, let me try to set it up. The challenge of this approach is that all background threads are stopped immediately when the process exits, without doing any cleanup. So we need some kind of handle that leads to a proper stop on drop.

If at the end of the day. If this is too hard, let's just wrap start and stop and leave up for another PR

That's also fine with me.

@haixuanTao
Copy link
Collaborator

Ok, let me try to set it up. The challenge of this approach is that all background threads are stopped immediately when the process exits, without doing any cleanup. So we need some kind of handle that leads to a proper stop on drop.

That's true about the cleanup

@haixuanTao
Copy link
Collaborator

haixuanTao commented Aug 2, 2024

Ok, let me try to set it up. The challenge of this approach is that all background threads are stopped immediately when the process exits, without doing any cleanup. So we need some kind of handle that leads to a proper stop on drop.

That's true about the cleanup

I guess we should ask users to use the destroy command before closing the script they are using.

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.

2 participants