Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Allow different names for IML CLI #2305

Merged
merged 1 commit into from
Oct 6, 2020
Merged

Allow different names for IML CLI #2305

merged 1 commit into from
Oct 6, 2020

Conversation

ip1981
Copy link
Member

@ip1981 ip1981 commented Oct 5, 2020

  • Determine executable name.

  • Remove "IML" where possbile.

Screenshot_2020-10-06_15-21-03

Separate packages would create a hard link for the executable and symlinks for shell completions.

Closes #2295.

Signed-off-by: Igor Pashev pashev.igor@gmail.com


This change is Reviewable

@ip1981 ip1981 requested a review from a team October 5, 2020 15:47
@ip1981 ip1981 self-assigned this Oct 5, 2020
#[derive(Debug, StructOpt)]
#[structopt(name = "iml", setting = structopt::clap::AppSettings::ColoredHelp)]
/// The Integrated Manager for Lustre CLI
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not come up with a generic non-trivial description :)

@ip1981 ip1981 changed the title Alow different names for IML CLI Allow different names for IML CLI Oct 5, 2020
iml-manager-cli/src/main.rs Outdated Show resolved Hide resolved
@ip1981 ip1981 marked this pull request as draft October 5, 2020 15:56
@ip1981 ip1981 marked this pull request as ready for review October 5, 2020 16:00
@jgrund
Copy link
Member

jgrund commented Oct 5, 2020

General question: what happens if the structopt name does not match the binary name?

I.E. if we had a binary called iml still but did something like #[structopt(name = get_env_name())... within the code, where get_env_name() resolved to foo

@ip1981
Copy link
Member Author

ip1981 commented Oct 5, 2020

General question: what happens if the structopt name does not match the binary name?

I think nothing. The binary name is totally unrelated to the actually running code.
Under the hood structopt just calls some Clap functions. Previously it was using a static string, but actually it can execute any code which deliver a string, for example reading env var or using OS-specific hacks to get the binary's name.

@jgrund
Copy link
Member

jgrund commented Oct 5, 2020

I think nothing. The binary name is totally unrelated to the actually running code.

So in the case above it would print foo for the help text?

@ip1981
Copy link
Member Author

ip1981 commented Oct 5, 2020

I think nothing. The binary name is totally unrelated to the actually running code.

So in the case above it would print foo for the help text?

Yes.

@jgrund
Copy link
Member

jgrund commented Oct 5, 2020

I think nothing. The binary name is totally unrelated to the actually running code.

So in the case above it would print foo for the help text?

Yes.

Ok, so in that case we will want to have an environment variable to name the cli. It should default to iml if it's not present and can be used for naming both iml and iml-config cli (with the iml prefix being the common part).

iml-manager-cli/src/lib.rs Outdated Show resolved Hide resolved
@ip1981 ip1981 marked this pull request as draft October 5, 2020 20:15
@ip1981
Copy link
Member Author

ip1981 commented Oct 5, 2020

So it seems impossible without actually renaming the executable (or doing something dirty):

Screenshot_2020-10-05_22-14-42

@jgrund
Copy link
Member

jgrund commented Oct 5, 2020

@jgrund
Copy link
Member

jgrund commented Oct 5, 2020

Can we reach into clap and set .bin_name(...)?

@ip1981
Copy link
Member Author

ip1981 commented Oct 6, 2020

Can we reach into clap and set .bin_name(...)?

This will be

    let matches = App::from_clap(
        &App::clap()
            .bin_name("fffffffffff")
            .name("wwwwwww")
            .get_matches(),
    );

Screenshot_2020-10-06_13-07-14

But if we don't rename the binary and use some script, e. g.

export CLI_NAME=foo
exec /usr/bin/iml "$@"

we may have some issues anyway, for example:

  1. process list
  2. core dumps

@ip1981 ip1981 force-pushed the ip1981/2295-foo-cli branch 2 times, most recently from 2300b58 to 95d693a Compare October 6, 2020 13:22
@ip1981 ip1981 marked this pull request as ready for review October 6, 2020 13:22
mkpankov
mkpankov previously approved these changes Oct 6, 2020
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r1, 8 of 8 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ip1981)


iml-manager-cli/src/config-main.rs, line 28 at r3 (raw file):

    iml_tracing::init();

    let name = selfname(Some("config")).unwrap_or_else(|| "iml-config".to_string());

Would it make sense to move the default into selfname so that it concatenates iml- to whatever comes in Option?


iml-manager-cli/src/lib.rs, line 32 at r2 (raw file):

Previously, ip1981 (Igor Pashev) wrote…

What about this:

pub fn selfname(suffix: Option<&str>) -> Option<String> {
// get var
// build with optional suffix (specific to the executable)
// default to executable name
}

I like

jgrund
jgrund previously approved these changes Oct 6, 2020
    - Determine executable name.
    - Remove "IML" where possbile.

Signed-off-by: Igor Pashev <pashev.igor@gmail.com>
@jgrund jgrund merged commit ad6daad into master Oct 6, 2020
@jgrund jgrund deleted the ip1981/2295-foo-cli branch October 6, 2020 17:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants