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

[RSDK-8422] add make commands for building installer #289

Merged

Conversation

gvaradarajan
Copy link
Member

No description provided.

@gvaradarajan gvaradarajan requested a review from mattjperez August 6, 2024 17:28
@gvaradarajan gvaradarajan requested a review from a team as a code owner August 6, 2024 17:28
Makefile Outdated
cargo build -p micro-rdk-installer --bin micro-rdk-installer --release

install-tools:
cargo install cargo-generate espflash espup cargo-espflash ldproxy; espup install -s -f ~/esp/export-rs.sh -v 1.67.0
Copy link
Member

Choose a reason for hiding this comment

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

  • This has always confused me: why is there both espflash and cargo-espflash as cargo installables?
  • I don't recall ever manually installing ldproxy. I think espup installs an ldproxy actually, and I don't think it is the same one as the ldproxy crate, though I'm not sure. For me at least, trying to install ldproxy after espup gets me :error: binary ldproxy already exists in destination.
  • I think our rust of record is newer than 1.67. I think it is 1.75 now.
  • You don't need to separate with ;, you can have multiple commands, one per line.
  • I'm not sure the Makefile should be overwriting a file in the users home directory, and I think this will fail with an error if ~/esp doesn't exist. Maybe emit it to cwd and log a message suggesting they copy it somewhere?

Meanwhile, it isnt' clear to me that it is actually necessary to source this file anymore. I haven't done it for months.

Copy link
Member

@mattjperez mattjperez Aug 6, 2024

Choose a reason for hiding this comment

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

In make build-esp32-bin we use cargo espflash save-image to compile a single binary that contains the partition table and program. A single artifact we can publish on GitHub.

Strangely, it doesn't appear that cargo-espflash can actually flash this single binary that it wrote. Here's an example of other users getting confused. Note that they too end up using espflash, though they actually point to the elf file in the target/release subfolder.

In make flash-esp32-bin, we are using espflash write-bin to write the complete binary to the device from 0x0. write-bin lets us write an arbitrary binary to an offset, not requiring an ELF file the way espflash flash does.

We actually use write-bin in micro-rdk-installer under the hood via the espflash crate for write-flash, update-app-image, and write-credentials. This makes our installer's behavior closer to to the current workflow compared to espflash flash or cargo espflash flash.

@gvaradarajan gvaradarajan requested a review from acmorrow August 7, 2024 16:46
@npmenard
Copy link
Member

npmenard commented Aug 7, 2024

I don't think we should take the burden of automating the installation of the tools it means we should also deal with updates, dependency handling, different platform etc... I believe this is more of a documentation problem where we can give general instructions while pointing to the relevant tool descriptions.
Generally devs should be using thew docker image initially until they are sufficiently frustrated to do their own environment setup

Makefile Outdated
build-installer:
cargo build -p micro-rdk-installer --bin micro-rdk-installer --release

install-tools:
Copy link
Member

Choose a reason for hiding this comment

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

It sounds like we should remove the install-tools target and only retain `build-installer.

@gvaradarajan
Copy link
Member Author

I agree with @npmenard and @acmorrow on removing the tool installations from the Makefile, but I'd like to give @mattjperez a chance to respond with any objections as the creator of the original ticket

@mattjperez
Copy link
Member

I agree with @npmenard and @acmorrow on removing the tool installations from the Makefile, but I'd like to give @mattjperez a chance to respond with any objections as the creator of the original ticket

Maybe I'm not thinking of it the right way, but "automating the installation of the tools" was kinda what we did with the new esp-idf setup. leveraging rust and cargo to handle the host's dependecies,etc seems similar in how we (now) rely on embed to compile to the user's host system and install everything for building esp-idf.

I imagined the docs (since we can build locally now) for a new user eventually being more like: install rust -> install cargo-generate -> generate project template -> make install-tooling -> make build-esp32-bin.

If the consensus is this doesn't make sense or add value, kill it.

@gvaradarajan
Copy link
Member Author

I think adding clarifications to docs for the idiosyncrasies that can be present in various systems when trying to install tooling is easier than trying to account for all scenarios in a make command, so I'm proceeding to remove

@gvaradarajan gvaradarajan changed the title [RSDK-8422] add make commands for building installer and dev tools [RSDK-8422] add make commands for building installer Aug 8, 2024
@gvaradarajan gvaradarajan requested a review from acmorrow August 8, 2024 14:28
@gvaradarajan gvaradarajan merged commit 1a05b88 into viamrobotics:main Aug 8, 2024
6 checks passed
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.

4 participants