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

feat: ic binary dependencies are included in tarball #39

Merged
merged 6 commits into from
Aug 4, 2023

Conversation

smallstepman
Copy link
Contributor

@smallstepman smallstepman commented Aug 2, 2023

instead of relying on IC binaries that are stored in dfx's cache, we're now packaging the dependencies inside the nns and sns extension tarballs. This allows teams responsible for these extension, to bump the version of these dependencies without the need to wait until they'll be bumped in dfx.

note: e2e tests are not covering this change, because they're relying on dfx extension install mechanism, which grabs the tarball from github releases. We should find ways to address this issue in a separate PR addressed in #40

@smallstepman smallstepman requested review from a team as code owners August 2, 2023 17:51
@ericswanson-dfinity
Copy link
Member

note: e2e tests are not covering this change, because they're relying on dfx extension install mechanism, which grabs the tarball from github releases. We should find ways to address this issue in a separate PR

Do the current e2e tests, for a PR, verify the functionality of the extension as modified in the PR? Or the functionality of the latest released extension?

@@ -22,33 +22,27 @@ teardown() {
}

@test "ic-nns-init binary exists and is executable" {
dfx cache install
Copy link
Contributor

Choose a reason for hiding this comment

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

So with this change in the e2e test we can be sure that the extensions aren't accidentally using the dfx bundled binaries?

Copy link
Contributor Author

@smallstepman smallstepman Aug 4, 2023

Choose a reason for hiding this comment

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

I removed dfx cache installs because, for some reason, it was overwriting the content of dfx cache directory, and subsequently, it was deleting the extensions directory, effectively removing the nns extension that was installed during setup().


Not sure what's the reasoning behind it, but after quick investigation this is indeed what it does:

@dfinity/sdk perhaps we should add --force flag to dfx cache install, so that users won't wipe their installed extensions when trying to fix their dfx cache?

Marcin Nowak-Liebiediew and others added 2 commits August 4, 2023 20:55
Co-authored-by: Daniel Thurau <[email protected]>
Co-authored-by: Daniel Thurau <[email protected]>
Copy link
Contributor

@DanielThurau DanielThurau left a comment

Choose a reason for hiding this comment

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

Left a few comments (some informational for me as I'm new to the code base) but overall LGTM 🚀

@smallstepman smallstepman merged commit 54376f7 into main Aug 4, 2023
std::env::current_exe().map_err(|e| anyhow::anyhow!("Failed to get current exe: {}", e))?;
let extension_dir_path = extension_binary_path
.parent()
.unwrap_or_else(|| Path::new("."));
Copy link
Member

Choose a reason for hiding this comment

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

In what circumstances would the extension binary path not have a parent directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I can't think of any, I'll just unwrap then

// The same applies to other bundled binaries.
command.env(
"PATH",
binary_to_call.parent().unwrap_or_else(|| Path::new(".")),
Copy link
Member

Choose a reason for hiding this comment

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

  1. is binary_to_call.parent() the same as extension_dir_path?
  2. where is the path to dfx coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is binary_to_call.parent() the same as extension_dir_path?

yes, thank you

where is the path to dfx coming from?

Why would it need a path to dfx? I think we're not concerned with dfx at this point because we've already localized the extension:

  1. user calls dfx EXT
  2. dfx version n.n.n could redirect execution to dfx version o.o.o (if we're in dir with dfx.json that specifies dfx version)
  3. dfx executes extension BIN in
    • .cache/dfinity/versions/o.o.o/extensions/EXT/BIN , or
    • .cache/dfinity/versions/n.n.n/extensions/EXT/BIN
  4. BIN does not care if it's in o.o.o/extensions/EXT or n.n.n/extensions/EXT. It only cares if it can find binaries in the same dir as BIN is in (aka std::env::current_exe())

extensions-utils/src/download_dependencies.rs Outdated Show resolved Hide resolved
extensions-utils/src/download_dependencies.rs Show resolved Hide resolved
extensions/nns/src/install_nns.rs Show resolved Hide resolved
// (downloaded binary name, renamed binary name)
("ic-nns-init", "ic-nns-init"),
("ic-admin", "ic-admin"),
("sns", "sns-cli"),
Copy link
Member

Choose a reason for hiding this comment

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

What do you think we should do about both the nns and sns extensions having a dependency on the sns binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Daniel has already explored a way to reproduce the same functionality without needing to download sns-cli : feat: ic binary dependencies are included in tarball #39 (comment). The work would come in a separate PR
  2. to answer this question more generally: the extension manifest has a field .dependencies. This field is currently not used, however, in the future, we could use this field to solve such issues.
    For example, nns extension depends on sns extensions; therefore, assuming we're in CACHE/extensions/nns/ directory, we could find the sns-cli binary in ../sns/sns-cli. I'm aware it's not that simple (e.g. what if someone already had sns ext install, or even worse, someone did dfx ext install azle --install-as sns) - more thinking is needed here
  3. for now, I'm not too concerned about this duplication, even tho I don't like it

@@ -46,5 +50,4 @@ pre-release-replacements = [
]

[package.metadata.dist]
include = ["extension.json"]

include = ["extension.json", "sns-cli", "ic-admin", "ic-nns-init"]
Copy link
Member

Choose a reason for hiding this comment

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

What would you think about putting all bundled binaries into a single subdirectory? We are setting precedent for other extensions, and recall dfx extension install --install-as.

Copy link
Contributor Author

@smallstepman smallstepman Aug 7, 2023

Choose a reason for hiding this comment

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

cargo-dist's support for including directories is TBD: https://opensource.axo.dev/cargo-dist/book/config.html#include

I don't know if you mean: put all bundled binaries in

  • CACHE/extensions/bundled-binaries
  • or in CACHE/extensions/EXT/bundled-binaries

?

I'm fine with the second, but I don't like the first because it would prevent developers from installing multiple versions of the extensions

extensions-utils/src/download_dependencies.rs Show resolved Hide resolved
"--url",
&opts.nns_url,
"--wasm-dir",
opts.wasm_dir.to_str().unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

We went over this a lot in a previous PR. Can you think of a way to do this that does not involve a lossy conversion on a path?

Comment on lines +493 to +494
.display()
.to_string();
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use .display() when passing a PathBuf as an argument. Can you think of a way to go through command.arg(), which does not have this problem?

ericswanson-dfinity added a commit that referenced this pull request Aug 4, 2023
@smallstepman smallstepman deleted the mnl/extensions-include-ic-binaries-in-tarball branch August 29, 2023 18:04
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.

3 participants