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

Pass the command for a job as an argument to bacon #284

Closed
c-git opened this issue Dec 24, 2024 · 26 comments · Fixed by #296
Closed

Pass the command for a job as an argument to bacon #284

c-git opened this issue Dec 24, 2024 · 26 comments · Fixed by #296

Comments

@c-git
Copy link
Contributor

c-git commented Dec 24, 2024

This is by no means a need but I've look and can't see that anyone asked before. I wanted to know if supplying the command job details to run on the command line would be something you'd be amenable to. So you could run one off jobs for example (I know cargo test has a built in job just didn't have a better idea for the example).

bacon -c "cargo test"

would be equivalent to having a bacon.toml like this:

[jobs.one-off]
command = ["cargo", "test"]

and then running

bacon -j one-off
@Canop
Copy link
Owner

Canop commented Dec 24, 2024

Problem is there are usually more parameters for a job, and it's only growing with bacon being used on non cargo tools.

@c-git
Copy link
Contributor Author

c-git commented Dec 24, 2024

Sorry I did think about there being many parameters and 100% forgot to include how I feel that could be addressed along with why I think this feature is useful in the first place.

Many job parameters

I considered 3 solutions to this problem. I'll list them in order of my preference but you have a better idea of the big picture than I do so feel free to not pick number 1 or none if you see it's a bad idea in a way I don't. (I'm also willing to try to implement it if time is the issue because I think it adds sufficient value, more on the value in the motivation.)

1 - Pass full job as one argument in json

Instead of adding a flag for each parameter just allow the user to pass all of them as a json string. I imagine it working by using -C to pass a full job config. And -c would be to pass just the command and use the defaults for the rest. Note the specific text format is not important just used json to explain as that works well all on one line. So for example you could call it with just the command

bacon -c "cargo test"

or if you need to change other settings

bacon -C '{"command": ["cargo", "test"], "need_stdout": true}'

For implementing the feature, I'm imagining that the string could be deserialized as a job assuming that's what happens right now with the toml. But either way what ever is done now for the toml try to do something similar.

2 - Only support common parameters

Alternatively only popular parameters could be supported and if you need to go outside of that then you need to create a bacon.toml as is needed now.

3 - Only support command with defaults

This is the simplest but also most limiting. And I suspect it would actually generate more complaints than value.

Why do this at all (MOTIVATION)

It's much easier to share a command to run than to give instructions on how to insert a job into a bacon.toml if you are trying to help someone remotely. I also often find myself using one off tasks like just to test a command and right now that means creating a job to test it. It's easier now with the hot config reload but would be easier still with being able to just supply the job at the command line where I don't need to separate each argument in quotes and commas.

@c-git
Copy link
Contributor Author

c-git commented Jan 1, 2025

Is this something worth pursuing?

@Canop
Copy link
Owner

Canop commented Jan 1, 2025

I don't know. I'm afraid it would be too limiting and end up in the addition of tons of launch parameters. I had a similar experience in my current project at work, where I finally had to drop pure command-line launches and keep only file based tasks.

The job passed as JSON as you propose in 2 is interesting but typing JSON by hand on the command line is far from convenient or easy.

@Canop
Copy link
Owner

Canop commented Jan 1, 2025

BTW I'd like other bacon users to give their opinion.

@c-git
Copy link
Contributor Author

c-git commented Jan 1, 2025

Agreed. Would be great to get feedback from more users. I agree about json not being convenient to type and I agree the multiple launch parameters doesn't seem viable. I'm concerned that the json ones while easier to share would still be very long but I just completed Shuttle's Christmas Code Hunt and they shared long commands for curl and actually it wasn't that bad.

This is an example from the challenge for Day 5, the top part is the command and the bottom is the expected output

image

@alerque
Copy link

alerque commented Jan 1, 2025

My thoughts are:

  • This would be nice to have
  • A --command/-c flag with a single string argument for the command to run with all args is certainly a reasonable way to set this up. Alternatively a "run" or "on-off" subcommand that took the command arguments as final SPLAT argument (past a -- separator) would also work and be welcome by shell gurus.
  • JSON blobs in shell args is a non-starter in my book. Gross.
  • All the other stuff that could/would be needed should be regular arguments. I would argue that with or without this one off job feature, any and every option that can be set for a job in the config should be overridable with CLI args anyway. Yes this means quite a few arguments. No I don't think that's a bad thing.

@Canop
Copy link
Owner

Canop commented Jan 1, 2025

Let's be concrete to better judge, starting with the current list of mission arguments.

The arguments of a job are defined here: https://github.com/Canop/bacon/blob/main/src/jobs/job.rs

Ignoring the arguments that are already present as launch arguments, here's the list of what's missing if we want to define any job at the command line, with my opinion regarding their necessity level.

  • allow_failures: essential
  • allow_warnings: essential
  • analyzer: essential
  • apply_gitignore: optional
  • background: essential
  • command: essential
  • default_watch: nice to have
  • env: optional (we can pass env vars otherwise)
  • expand_env_vars: optional
  • extraneous_args: not needed
  • ignore: optional
  • ignored_lines: probably optional
  • kill: essential
  • need_stdout: essential
  • on_change_strategy: essential for long running programs
  • on_success: not necessary for cmd call

I don't think I want to dilute existing launch arguments with such profusion and I really don't want to have to tell users "this one liner doesn't work because you didn't specify background=false or analyzer=nextest which you can't because it's not a launch argument").

@Canop
Copy link
Owner

Canop commented Jan 1, 2025

Passing commands with the equivalent of --data looks like a good idea both to avoid 20 more launch arguments and to allow more complex setups, eg with several jobs. I would probably call that --conf-toml to prepare for the support of eg Hjson and JSON.

Maybe we could also pass the config file on stdin.

@Canop
Copy link
Owner

Canop commented Jan 1, 2025

@alerque Right now, "the command arguments as final SPLAT argument (past a -- separator)" are given to the executed command

@c-git
Copy link
Contributor Author

c-git commented Jan 3, 2025

Another option I hadn't thought of (and am not sure it's a great idea) is to derive clap::Args for the struct that stores the jobs. That way it will automatically create flags for each of the fields and put them all under a sub command. I tried to copy the actual relevant types from bacon, Args and Jobs but they were too long and I also didn't want to invest too much time into just showing the idea as it may not even be a good fit. I also enabled the default help so I could show it in the example but I suspect that won't work for bacon in practice so there might be even more overhead to going with this approach.

Output of help on top level binary

$ cargo run -- -h
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.04s
     Running `target/debug/bacon -h`
Launch arguments

Usage: bacon [OPTIONS] [COMMAND]

Commands:
  inline  Pass the job to be run on the command line
  help    Print this message or the help of the given subcommand(s)

Options:
      --prefs    Print the path to the prefs file, create it if it doesn't exist
  -h, --help     Print help
  -V, --version  Print version

Output of help on the subcommand

$ cargo run -- inline -h
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/bacon inline -h`
Pass the job to be run on the command line

Usage: bacon inline [OPTIONS]

Options:
      --allow-failures       Whether to consider that we can have a success when we have test failures
      --allow-warnings       Whether to consider that we can have a success when we have warnings. This is
                             especially useful for "cargo run" jobs
      --analyzer <ANALYZER>  The analyzer interpreting the output of the command, the standard cargo dedicated one
                             if not provided
  -h, --help                 Print help

Code used (plus clap in the cargo toml)

use clap::{command, Args as ClapArgs, Parser, Subcommand};

/// Launch arguments
#[derive(Debug, Parser)]
#[command(
    author,
    about,
    version,
    // disable_version_flag = true,
    // disable_help_flag = true
)]
pub struct Args {
    /// Print the path to the prefs file, create it if it doesn't exist
    #[clap(long)]
    pub prefs: bool,

    #[command(subcommand)]
    command: Option<Command>,
}

#[derive(Debug, Subcommand)]
#[command()]
enum Command {
    /// Pass the job to be run on the command line
    #[command()]
    Inline(Job),
}

/// One of the possible job that bacon can run
#[derive(ClapArgs, Debug, Clone)]
pub struct Job {
    /// Whether to consider that we can have a success
    /// when we have test failures
    #[clap(long)]
    pub allow_failures: bool,

    /// Whether to consider that we can have a success
    /// when we have warnings. This is especially useful
    /// for "cargo run" jobs
    #[clap(long)]
    pub allow_warnings: bool,

    /// The analyzer interpreting the output of the command, the
    /// standard cargo dedicated one if not provided
    #[clap(long)]
    pub analyzer: Option<String>,
}

fn main() {
    let args = Args::parse();
    dbg!(args);
}

@Canop
Copy link
Owner

Canop commented Jan 3, 2025

IMO the cleanest solution now is the --data one. It's only one parameter and would only require a few added lines of code to support. The main questions are "is this convenient?", "is this useful?", and "how should this launch parameter be called?".

I suppose we should add it as an experiment to answer at least the 2 first questions.

@c-git
Copy link
Contributor Author

c-git commented Jan 3, 2025

IMO the cleanest solution now is the --data one. It's only one parameter and would only require a few added lines of code to support.

And importantly it doesn't require more code to support it as features get added. It's the one with the lowest maintenance burden which IMO is very important. And honestly from my point of view as a user it's the easiest to understand. I can reuse what I would have put in the toml file and don't need to consult the help for each field I want to add.

The main questions are "is this convenient?",

It's not the most convenient because it requires longer commands than the feature flag option but that said I think the maintenance trade off makes it worthwhile and will be easier to read than the feature flag option. So IMO it's the most balanced in terms of convenience, easier to read and easier to maintain. Just harder to type but it's typed once and maintained and read repeatedly.

"is this useful?", and

Personally I think it is very useful. I have to write up some instructions on how to use bacon and it's much easier to give one command even if it's long compared to instructions on explaining how to setup a bacon toml, how to set the command to run, how to set the needed fields, then how to run the job.

"how should this launch parameter be called?".

This I'm not sure about, personally I like that "data" is short but I wouldn't guess that's what it does. I like inline_toml or something like that but that's very long.

I suppose we should add it as an experiment to answer at least the 2 first questions.

I think that's a great idea but I think we should not include it in the default install and rather add an experimental feature flag so users can opt into it when installing but it's not the default so we can remove it without unnecessary breakage.

@Canop
Copy link
Owner

Canop commented Jan 3, 2025

I'll add it, then.

Regarding the name, I'd like to convey that it's another config element, equivalent to the various prefs.toml and bacon.toml files, and that it's in toml so that bacon can more easily support other configuration languages in the future.

@c-git
Copy link
Contributor Author

c-git commented Jan 3, 2025

Yes, I agree. To confirm it would have the highest priority so just above the bacon.toml?

@Canop
Copy link
Owner

Canop commented Jan 3, 2025

Yes, just before other command line arguments

@c-git
Copy link
Contributor Author

c-git commented Jan 3, 2025

Yes, just before other command line arguments

I can't figure out what difference the order of the arguments makes? Is it a restriction imposed by Clap, I though unless arguments were positional it didn't matter.

@Canop
Copy link
Owner

Canop commented Jan 3, 2025

I'm speaking of priorities: I would apply the settings of the --data thing before applying the other launch arguments (which would then override it)

@c-git
Copy link
Contributor Author

c-git commented Jan 3, 2025

oh, I see. Yes that makes sense. So it would be treated like a higher priority bacon.toml that gets overridden on the command line. Ok that makes sense.

Canop added a commit that referenced this issue Jan 11, 2025
@Canop
Copy link
Owner

Canop commented Jan 11, 2025

Can you check #296 and try evaluate whether it answers the need ?

@c-git
Copy link
Contributor Author

c-git commented Jan 11, 2025

Thank you I will check it.

@Canop
Copy link
Owner

Canop commented Jan 11, 2025

A downside of the chosen solution is that if you specify a job, and it's not the default job, it's not the one executed, which may be surprising for who isn't used to the multi job system of bacon.

Canop added a commit that referenced this issue Jan 15, 2025
@arifd
Copy link

arifd commented Jan 22, 2025

Something to consider is whether this need actually fits within the scope of bacon, before you blow up complexity of bacon to fit all needs.

I must admit, I am here only because I switched to bacon from cargo watch, and right now, all i really want to do is have something with the simplicity of cargo watch -s.

...Because I have a shell script that handles a custom lint that clippy doesn't have. and i'm doing a one-time fixing up of my codebase before it becomes enforced by CI. I keep finding a line to fix, fixing the line then running the script again. It's not worth writing a Config.toml and/or learning crazy CLI syntax in order to achieve this.

I originally thought, duh this is a no-brainer for bacon, but maybe bacon is not trying to be a file watcher?

EDIT: oh lol. only just saw that this issue has been closed and completed!

@Canop
Copy link
Owner

Canop commented Jan 22, 2025

EDIT: oh lol. only just saw that this issue has been closed and completed!

No worry.

BTW if you feel like bacon doesn't fit the bill or should be improved, I'd welcome you input on miaou.

@c-git
Copy link
Contributor Author

c-git commented Jan 22, 2025

EDIT: oh lol. only just saw that this issue has been closed and completed!

Did you figure out how to do what you were trying to do? There is an example in the cookbook of how to run a script https://dystroy.org/bacon/cookbook/#bacon-cli-snippets . The example actually runs echo but you can replace as needed.

@arifd
Copy link

arifd commented Jan 24, 2025

Sorry for taking so long to reply. I didn't use bacon in the end because as soon as I remembered i had cargo watch installed, I just went with what I knew.

I looked at the link, glad to see it's just toml in the CLI, so no need to learn anything knew.

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 a pull request may close this issue.

4 participants