Merge binaries into main probe-rs crate.

[Dirbaio]

TODO:

• Make the cargo-flash/cargo-embed shim binaries work on Windows
• Fix "unused" warnings

[noppej]

@Dirbaio ... This file rename: probe-rs/cli/src/debugger.rs -> probe-rs/src/bin/probe-rs/debugger.rs

Does it make sense to introduce a subfolder probe-rs/src/bin/cli that can hold all the files associated with the current and future CLI capabilities (e.g. benchmark, debugger, trace, run, etc.)?

[Dirbaio]

Yes, that's sort of what I'm doing. The main probe-rs binary is at src/bin/probe-rs, all the CLI functionality is modules under it.

[noppej]

Yes, that's sort of what I'm doing. The main probe-rs binary is at src/bin/probe-rs, all the CLI functionality is modules under it.

Looking at the final directory structure, it feels like there is an opportunity to move all the src/bin/probe-rs/*.rs except for main.rs into a src/bin/probe-rs/cli folder. Not sure I'm explaining it properly, but it feels like the cli functionality can grow over time, just like the other folders under src/bin/probe-rs, so keeping it out of the root of that folder might keep it neater. Just a thought :)

PS. There are some list and list-chips capabilities in the dap-server, which is now provided by the integrated binary. I'm happy to PR that cleanup in the future if you prefer.

[noppej]

At first, I thought there was no way to cargo install the dap-server, but then I figured out that the cargo install --features="cli" will install the probe-rs binary that includes the dap-server sub command.

I'm wondering if it makes sense to make that the default feature? Or else perhaps update the error message for cargo install, which currently gives you

warning: none of the package's binaries are available for install using the selected features
  bin "probe-rs" requires the features: `cli`
  bin "cargo-flash" requires the features: `cli`
  bin "cargo-embed" requires the features: `cli`
  example "nrf53_recover" requires the features: 
  example "ram_download" requires the features: 
  example "multidrop_raw" requires the features: 
  example "tcp_itm" requires the features: 
1 more targets also requires features not enabled. See them in the Cargo.toml file.
Consider enabling some of the needed features by passing, e.g., `--features="cli"`

To help users understand that the --features="cli" will enable gdb and dap servers also.

[noppej]

I'm not sure how to add this to review, but in the file .vscode/launch.json, can you please update the entry for testing the DAP server, to look like this (reflect the new binary names):

    {
      "type": "lldb",
      "request": "launch",
      "name": "DAP-Server probe-rs", // Use this to test the dap server .
      "cargo": {
        "args": ["build", "--bin=probe-rs", "--features=cli"],
        "filter": {
          "name": "probe-rs",
          "kind": "bin"
        }
      },
      "args": ["dap-server", "debug", "--port", "50001"],
      // "env": {
      //     "RUST_LOG": "error",
      //     "DEFMT_LOG": "info"
      // },
      "cwd": "${workspaceFolder}/probe-rs/src/bin/probe-rs/dap_server"
    }

[Dirbaio]

Looking at the final directory structure, it feels like there is an opportunity to move all the src/bin/probe-rs/*.rs except for main.rs into a src/bin/probe-rs/cli folder. Not sure I'm explaining it properly, but it feels like the cli functionality can grow over time, just like the other folders under src/bin/probe-rs, so keeping it out of the root of that folder might keep it neater. Just a thought :)

I'm not sure. Everything under src/bin/probe-rs is already "the CLI". There's the main.rs file, and then all subcommands are already child mods of that. We'd be adding a cli mod that's empty if we did this. IMO the paths are already long enough.

PS. There are some list and list-chips capabilities in the dap-server, which is now provided by the integrated binary. I'm happy to PR that cleanup in the future if you prefer.

Yes, for now I've focused on merging everything as-is and not doing any cleanup/reorganization to make review easier/faster. There's lots of stuff to clean up, I was planning on doing it in future PRs.

At first, I thought there was no way to cargo install the dap-server, but then I figured out that the cargo install --features="cli" will install the probe-rs binary that includes the dap-server sub command.

The idea of the merge is that the user doesn't "install the DAP server" or "install cargo-embed". They "install probe-rs", that's it. It's a single thing with all the functionality.

I'm wondering if it makes sense to make that the default feature?

Making it default means everyone using the probe-rs crate as a lib will get all the CLI deps in their dep tree, which add quire a bit of build time. (Yes, users could do default-features = false to avoid it, but they WILL forget to do it).

I agree having to set the feature is not great UX. However, I'm not too worried about the cargo install UX, because we will provide binary builds in the future, everyone will install using that.

Or else perhaps update the error message for cargo install, which currently gives you

That error message is not customizable. :(

To help users understand that the --features="cli" will enable gdb and dap servers also.

I don't think we should make the user worry about "getting the DAP server", they should just worry about "installing probe-rs". ie the vscode extension docs could tell the user "Install probe-rs, then install the probe-rs vscode extension. The extension uses the system-wide probe-rs installation".

[noppej]

The idea of the merge is that the user doesn't "install the DAP server" or "install cargo-embed". They "install probe-rs", that's it. It's a single thing with all the functionality.

Totally understood and the logic about keeping the api dep tree clean. In that case, do we want to call that feature "cli" (the feature name sent me down the path of misunderstanding, because I didn't see 'server' components falling under 'cli')? Perhaps something like "tools"?

[Dirbaio]

It's not "tools", it's THE probe-rs tool :) IMO cli is fine. Perhaps bin? Dunno, I'm fine with whatever name you guys choose.

[noppej]

I will leave it up to tiwalun and yatekii to decide. I was just trying to help make sure new users don't get too confused when trying to get off the ground with probe-rs.

[noppej]

Refactoring is never fun, but it feels like this sets us on the right path ... thanks :)

[Tiwalun]

Looks good, thanks for all the work!

Just some small nits, but nothing big regarding the code.

I think we should update the README as well, now it seems that all the installation instructions which were part of the individual readmes are gone. Could also be done in a separate PR, but then we should create an issue so it doesn't get forgotten.

[Tiwalun]

@Dirbaio @MabezDev How do you want to handle this and #1629 ? Not sure what order of merging the PRs is easier.

[MabezDev]

Merge this first I think, my rebase should be pretty straightforward.

[Dirbaio]

Rebased, resolved @Tiwalun 's comments (except one)

[Tiwalun]

Thanks! ❤️