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

install avalanchego & subnetevm via avalanche-cli #392

Closed
wants to merge 4 commits into from

Conversation

feuGeneA
Copy link
Contributor

@feuGeneA feuGeneA commented Jun 3, 2024

Why this should be merged

This eliminates the scripts in this repo that download and install avalanchego and subnetevm.

How this works

It uses avalanche-cli to download those things for us.

How this was tested

The e2e tests in CI now run with the avalanchego and subnetevm binaries downloaded and installed by CI.

How is this documented

In the root README.md and in .github/workflows/test.yml.

@feuGeneA feuGeneA force-pushed the feuGeneA/bin-deps-via-ava-cli branch from 3d955f0 to c126f00 Compare June 4, 2024 14:45
Signed-off-by: F. Eugene Aumson <[email protected]>
Comment on lines 21 to 36
export AVALANCHEGO_BUILD_PATH=${AVALANCHE_CLI_DIR}/bin/avalanchego/avalanchego-${AVALANCHEGO_VERSION}

# ensure that avalanche-cli has installed the version of avalanchego that we need:
if [[ ! -x "$AVALANCHEGO_BUILD_PATH/avalanchego" ]]; then
avalanche network clean
avalanche network start --avalanchego-version "$AVALANCHEGO_VERSION"
avalanche network clean
fi

# ensure that avalanche-cli has installed the version of subnetevm that we need:
subnetevm_bin=${AVALANCHE_CLI_DIR}/bin/subnet-evm/subnet-evm-${SUBNET_EVM_VERSION}/subnet-evm
if [[ ! -x "$subnetevm_bin" ]]; then
avalanche subnet create dummy --evm --vm-version "$SUBNET_EVM_VERSION" \
--force \
--teleporter --relayer --evm-chain-id 1 --evm-token DUM --evm-defaults # this line is just to avoid the dialog prompts
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make the order of operations for these two consistent? I like
Path
Comment
if block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 5c48264

Copy link
Contributor

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

Does avalanche-cli have commands for directly installing the avalanchego and subnet-evm binaries? Manipulating the network just to install them seems like overkill and risks polluting or overwriting a user's avalanche-cli data.

I'm all for reducing the amount of bash we have to maintain, but am hesitant to do so if the alternative has additional overhead.

@feuGeneA
Copy link
Contributor Author

feuGeneA commented Jun 6, 2024

Does avalanche-cli have commands for directly installing the avalanchego and subnet-evm binaries? Manipulating the network just to install them seems like overkill and risks polluting or overwriting a user's avalanche-cli data.

I'm all for reducing the amount of bash we have to maintain, but am hesitant to do so if the alternative has additional overhead.

@cam-schultz No, avalanche-cli does not have commands for installing those binaries. (That would be ideal...)

Aat the time I submitted this I hadn't yet realized the distinction between avalanche-cli and avalanche-network-runner, and I was envisioning that we would be incorporating avalanche-cli usage into this repo anyways (especially given all the "migrate to avalanche-cli" wording in the issues tracked by #385). Given that perspective, I was thinking that, if we're going to be migrating to avalanche-cli anyways, then there'd be no hope of avoiding the polluting/overwriting of the user's avalanche-cli data anyways, so the scripts in this PR seemed acceptable in this regard.

Having said all that, I'm happy to close this PR if that's what makes sense to the team.

@feuGeneA
Copy link
Contributor Author

feuGeneA commented Jun 6, 2024

see also ava-labs/avalanche-cli#1938

Comment on lines +40 to +43
# This is a hack that can probably be undone when we stop setting up the
# avalanche network and subnet ourselves (via BeforeSuite/AfterSuite in
# tests/local/e2e_test.go) and start having avalanche-cli do that for us
# instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're going to use the tempnet fixture rather than avalanche-cli for use in the Ginkgo E2E tests, this comment should probably be updated.

@cam-schultz
Copy link
Contributor

I was envisioning that we would be incorporating avalanche-cli usage into this repo anyways

I think incorporating avalanche-cli directly could be overkill. It's a useful interface, but all we really need is the local network it spins up. The scenario I'm considering is one in which a user maintains a Fuji or mainnet subnet named dummy, and when they go to run the e2e tests, they either fail or their existing subnet info is corrupted.

@feuGeneA feuGeneA marked this pull request as draft June 11, 2024 22:04
@cam-schultz
Copy link
Contributor

We discussed this offline, and came to the conclusion that the ideal solution would be to add install scripts directly to subnet-evm and avalanchego, and leverage them directly in our repos. A standalone install command in the CLI would also work, but is less preferable because the CLI is downstream of teleporter, so leveraging that in our tests would create a circular dependency. Barring that, the consensus is that we should leave the install scripts as-is, rather than add the CLI as a dependency and potentially cause conflicts with other CLI usage.

@feuGeneA feuGeneA closed this Jun 11, 2024
@michaelkaplan13 michaelkaplan13 deleted the feuGeneA/bin-deps-via-ava-cli branch June 14, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants