Add ability to specify commit of avalanchego in ci #589

richardpringle · 2023-03-22T14:51:15Z

Why this should be merged

Resolves #500

How this works

use the AVALANCHE_VERSION environment variable to specify a commit or branch name (or tag without a prebuilt binary).

How this was tested

tested here for a commit:
https://github.com/ava-labs/subnet-evm/actions/runs/4512439095/jobs/7945887581

and here for a branch:
https://github.com/ava-labs/subnet-evm/actions/runs/4512412729/jobs/7945824005

How is this documented

documentation for the whole process needs to be added

darioush · 2023-03-22T18:10:51Z

scripts/install_avalanchego_release.sh

+    if [[ -d .git ]]; then
+      git fetch
+    else
+      git clone ${GIT_CLONE_URL} .


should we make this a shallow clone of the repository and avoid fetching the entire repository if the dir exists?

Yeah, good catch. I had that at one point, but I guess it got lost while I was moving some things around.

scripts/install_avalanchego_release.sh

aaronbuchwald · 2023-03-22T19:27:26Z

scripts/install_avalanchego_release.sh

+# TODO: Please read:
+# not sure why the version isn't included in CI currently... open to suggestions on how to handle this better
+AVALANCHEGO_BUILD_PATH=${AVALANCHEGO_BUILD_PATH-${VERSION}:-${BASEDIR}/avalanchego-${VERSION}}


is the question why do we install this in a directory without the version as a suffix?

Yeah... made me think that it's being used elsewhere.

Currently we use this as a utility in local development to install a release of AvalancheGo, but we use it more frequently in CI to download a specific release that Subnet-EVM depends on to run its e2e tests.

In CI, we populate this environment variable here: https://github.com/ava-labs/subnet-evm/blob/master/.github/workflows/ci.yml#L77 and then pass the same variable in to run our ginkgo e2e tests here: https://github.com/ava-labs/subnet-evm/blob/master/.github/workflows/ci.yml#L83 (we could definitely clean this up a bit).

Just noticed we actually do something else that is weird here which is we use BASE_DIR instead of BASEDIR in one place in the script (on master) here: https://github.com/ava-labs/subnet-evm/blob/master/scripts/install_avalanchego_release.sh#L21.

Running this script locally now I'm running into the issue that instead of installing in /tmp/ it's actually installing in the Subnet-EVM directory in the build path: v1.9.11:-/tmp/avalanchego-release/avalanchego-v1.9.11.

It looks like this is because the syntax ${var:-<alternative>} checks if var is populated and uses it if so, but otherwise uses <alternative>. Since this change is not using a single variable, it seems to just use the whole string v1.9.11:-/tmp/avalanchego-release/avalanchego-v1.9.11 instead of putting it in /tmp.

Okay, take a look now. I left the commit as a fixup and will squash once I get the green light. Either we keep these changes or I can figure out a better way to deal with the branching logic.

ceyonur · 2023-03-23T12:14:49Z

scripts/install_avalanchego_release.sh

 mkdir -p $AVALANCHEGO_BUILD_PATH
+mkdir -p $VERSIONED_AVALANCHEGO_BUILD_PATH


why do we need this as a different path?

I agree with this, I'd prefer to just use a single AVALANCHEGO_BUILD_PATH and either build in that location or unpack the downloaded release into that location rather than adding a separate versioned path.

@aaronbuchwald, why do you prefer a single build path?

@ceyonur, the reason I did this was actually to avoid further nesting of logic, making the whole script harder to read. Not the best reason, but a reason, none-the-less.

I'm now thinking that AVALANCHE_VERSION is too overloaded and that it would be better to separate things into AVALANCHE_RELEASE_VERSION, AVALANCHE_COMMIT, and AVALANCHE_BRANCH and the logic for each variant is completely different (branches need to pulled in every time).

@aaronbuchwald, I also think it makes a lot more sense to have versioned paths, the same way that cargo stores dependencies for instance. I can make changes such that we don't rebuild if an existing commit or release-tagged version already exists, and only rebuild for a specified branch.

Thoughts?

Sounds good to me 👍

ceyonur · 2023-03-23T12:20:36Z

scripts/install_avalanchego_release.sh

 mkdir -p $AVALANCHEGO_BUILD_PATH
+mkdir -p $VERSIONED_AVALANCHEGO_BUILD_PATH

 if [[ ! -f ${AVAGO_DOWNLOAD_PATH} ]]; then


I feel like we're checking the wrong path here. if we already extracted the downloaded/built binary to VERSIONED_AVALANCHEGO_BUILD_PATH i suppose we don't need to redownload/rebuild the binary for that version.

You're absolutely right for commits and binary-releases, but this doesn't apply for branches, which was specified in the original issue.

We should update this so that if a commit or branch name

(emphasis mine)

Branches can change, so if we're dealing with a commitish version, then we need to re-fetch and checkout.

This makes me think about a different flaw though... if we want to grab the latest updates of a branch, we need to checkout origin/<branch_name> instead.

I will revise to handle this case.

ceyonur · 2023-03-23T12:21:43Z

scripts/install_avalanchego_release.sh

-  mv ${AVALANCHEGO_BUILD_PATH}/build/* ${AVALANCHEGO_BUILD_PATH}
-  rm -rf ${AVALANCHEGO_BUILD_PATH}/build/
+
+if [[ ! -f ${VERSIONED_AVALANCHEGO_BUILD_PATH}/avalanchego ]]; then


this is the only path we should be checking. otherwise if this exists (and download path does not) we would just download it for nothing, right?

See my response here

ceyonur · 2023-03-23T12:30:57Z

scripts/install_avalanchego_release.sh

@@ -27,22 +27,72 @@ if [[ ${GOOS} == "darwin" ]]; then
  AVAGO_DOWNLOAD_PATH=${BASEDIR}/avalanchego-macos-${VERSION}.zip
 fi

-AVALANCHEGO_BUILD_PATH=${AVALANCHEGO_BUILD_PATH:-${BASEDIR}/avalanchego-${VERSION}}
+# TODO: Please read:
+# not sure why the version isn't included in CI currently... open to suggestions on how to handle this better


You mean why it's not being used or why is not working? I don't think we use any AVALANCHE_VERSION=xxx as an env var in CI currently. Subnet-EVM (or any other rpcchainVM) cannot work with all AvalancheGO versions. There is a protocl version handshake, and if AvalancheGo bumps that protocol version and older subnet-evm cannot work with it. Similarly a recent Subnet-EVM (with newer protocol version) won't work with an older AvalancheGo version. e2e tests takes around ~5 min. It's probably best to use latest possible avalanchego for Pull request CI (to reduce test time). However we can include "all supported" avalanchego versions in a release-CI.

It's probably best to use latest possible avalanchego for Pull request CI (to reduce test time). However we can include "all supported" avalanchego versions in a release-CI.

@aaronbuchwald, do you have any opinions here? This comment is really about the existence for #500 and whether or not that makes sense (if I'm not mistaken). The justification makes sense to me, but I'm too new to have worked on subnet-evm while in the process of releasing Avalanche-Go to regurgitate the motivation behind #500.

You mean why it's not being used or why is not working?

@ceyonur, thanks for answering! It actually is being used in the last step for the e2e tests in CI. I can actually remove the question now.

This is an interesting idea. As @ceyonur mentioned AvalancheGo and Subnet-EVM can only talk to each other if they are on the same RPCChainVMProtocolVersion.

AvalancheGo documents this for all releases here: https://github.com/ava-labs/avalanchego/blob/master/version/compatibility.json
Subnet-EVM documents this for all releases here: https://github.com/ava-labs/subnet-evm/blob/master/compatibility.json

I think the script (and whats in scope for this PR) should make sure that we can install any tag or commit-ish of AvalancheGo. In the future, your suggestion of moving this to a generally available install script in AvalancheGo and even a supported GitHub action would be really nice too.

As for what we run in CI, we could definitely add a check to CI that runs the Subnet-EVM e2e tests with every version of AvalancheGo that should be supported, which could just use this script (and actually would not depend on this change if we just wanted to run with official AvalancheGo releases). Although, I'd prefer this as a cronjob or manual GitHub action because it seems like overkill to run frequently given both the rpcchainvm server and client are in AvalancheGo, so I think it would make more sense to run this to confirm prior to releases or at a weekly cadence.

aaronbuchwald · 2023-03-23T13:57:37Z

.github/workflows/ci.yml

@@ -74,7 +74,7 @@ jobs:
        working-directory: ./contract-examples
      - name: Install AvalancheGo Release
        shell: bash
-        run: BASEDIR=/tmp/e2e-test AVALANCHEGO_BUILD_PATH=/tmp/e2e-test/avalanchego ./scripts/install_avalanchego_release.sh
+        run: BASE_DIR=/tmp/e2e-test AVALANCHEGO_BUILD_PATH=/tmp/e2e-test/avalanchego ./scripts/install_avalanchego_release.sh


Can we align all of these BASEDIR and BASE_DIR to be the same? The previous bug was that they were ever different as the intended behavior is to allow someone to populate the environment variable and use the default if not populated.

I don't mind doing that, but it looks like it was done on purpose as the pattern repeats itself in a couple of places, AVALANCHE_VERSION vs AVALANCHE_GO_VERSION for instance. BASEDIR is only meant to be used inside the script.

If I assume that using a "private" variable name like that is justified, I think it would be more pragmatic to check if the user has defined BASEDIR instead of BASE_DIR and error out with an explanation. I can do the same for AVALANCHE_GO_VERSION vs AVALANCHE_VERSION .

What do you think?

In this case, I think both should be public environment variables that can be easily overridden by the user and I'd prefer to be consistent in using the same environment variable in the script that the user needs to override rather than renaming in the background.

I think all of the renaming is confusing when looking at the script and wanting to know how to override a variable or where it comes from.

If we want to go back to having public vs. private env variables, I think we should be explicit and follow the convention of using lowercase for private env variables.

aaronbuchwald · 2023-03-24T01:51:17Z

scripts/versions.sh

+AVALANCHE_VERSION=${AVALANCHE_VERSION:-'v1.9.11'}
+AVALANCHEGO_VERSION=${AVALANCHEGO_VERSION:-$AVALANCHE_VERSION}


Suggested change

AVALANCHE_VERSION=${AVALANCHE_VERSION:-'v1.9.11'}

AVALANCHEGO_VERSION=${AVALANCHEGO_VERSION:-$AVALANCHE_VERSION}

AVALANCHEGO_VERSION=${AVALANCHEGO_VERSION:-'v1'.9.11'}

These are the same thing, can we make them one variable

Yeah, I was a little worried about AVALANCHE_VERSION being used in other scripts though. My change makes this backwards compatible, so you can change the other scripts one at a time and make sure you don't break anything. I think that would be be better as a follow up PR so the context is a little more clear.

So how about I do that immediately after this is merged?

Sounds good to me

aaronbuchwald · 2023-03-24T01:51:56Z

scripts/install_avalanchego_release.sh

-  unzip ${AVAGO_DOWNLOAD_PATH} -d ${AVALANCHEGO_BUILD_PATH}
-  mv ${AVALANCHEGO_BUILD_PATH}/build/* ${AVALANCHEGO_BUILD_PATH}
-  rm -rf ${AVALANCHEGO_BUILD_PATH}/build/
+extract_archive() {


😎 (why I can't do this as a reaction is beyond me)

scripts/install_avalanchego_release.sh

aaronbuchwald · 2023-03-24T01:57:27Z

scripts/install_avalanchego_release.sh

 mkdir -p ${BASEDIR}
+
+VERSION=$AVALANCHEGO_VERSION


Would it be better to use a single variable for this instead of VERSION and AVALANCHEGO_VERSION ?

Yeah, I didn't really mean to move this. I kept VERSION because it makes the next couple of lines shorter. Happy to change it though

scripts/install_avalanchego_release.sh

aaronbuchwald · 2023-03-24T02:27:32Z

scripts/install_avalanchego_release.sh

+    # use the commit hash instead of the branch name or tag
+    BUILD_DIR=${AVALANCHEGO_BUILD_PATH}-${COMMIT}


Hmm at first I thought that the below code where it builds iff the build dir does not exist may run into issues for branches if there were newly pushed changes, but I see that this ensures that the build dir is based off of the commit, so that's not an issue.

I'm brilliant, what can I say, 😉

scripts/install_avalanchego_release.sh

aaronbuchwald

LGTM

darioush · 2023-03-24T15:27:32Z

scripts/install_avalanchego_release.sh

+  mkdir -p ${BUILD_DIR}
+
+  if [[ ${AVAGO_DOWNLOAD_PATH} == *.tar.gz ]]; then
+    tar xzvf ${AVAGO_DOWNLOAD_PATH} --directory ${BUILD_DIR} --strip-components 1


should we drop v?

I don't think it's worth the change. Every time I make a change, I have to run CI three different times to make sure everything is working as expected (one for tagged version, one for branch, and one for commit). While I know that dropping a "verbose" option won't change any functionality, if I don't run those tests again, the screen shots and linked actions won't actually contain a commit in this branch.

Long winded way of saying, "if we want to make that change, it should happen in another PR".

darioush · 2023-03-24T15:34:29Z

scripts/install_avalanchego_release.sh

+
+    cd ${GIT_CLONE_PATH}
+
+    git fetch


I still think we shouldn't fetch or clone the entire repository to get a specific branch or commit.

What do you mean by "fetch or clone the entire repository"? To my knowledge, you can't partially fetch a repository (unless you have git submodules or you're using LFS or something like that).

From the git docs:

When no remote is specified, by default the origin remote will be used, unless there’s an upstream branch configured for the current branch.

Regardless, fetch will grab everything we need here, then we always checkout a detached-head by prefixing branch names with origin/<branch_name> or a commit directly.

Yes, however it also fetches other branches that are not needed for this run.
I think this is fine for now, in the longer term I think we should use github actions to cache or create artifacts

richardpringle requested a review from anusha-ctrl as a code owner March 22, 2023 14:51

richardpringle marked this pull request as draft March 22, 2023 14:52

richardpringle mentioned this pull request Mar 22, 2023

Add ability to specify commit of avalanchego in ci #587

Closed

richardpringle force-pushed the ci-avago-commitish branch from 6b2bdbd to bcc61a3 Compare March 22, 2023 16:07

richardpringle marked this pull request as ready for review March 22, 2023 16:44

richardpringle requested review from ceyonur, darioush and aaronbuchwald as code owners March 22, 2023 16:44

richardpringle force-pushed the ci-avago-commitish branch 2 times, most recently from dd414e9 to a1c98a7 Compare March 22, 2023 17:01

darioush reviewed Mar 22, 2023

View reviewed changes

richardpringle force-pushed the ci-avago-commitish branch from a1c98a7 to 99cf9f8 Compare March 22, 2023 19:13

aaronbuchwald reviewed Mar 22, 2023

View reviewed changes

richardpringle requested review from darioush and aaronbuchwald March 22, 2023 20:04

richardpringle marked this pull request as draft March 23, 2023 02:10

ceyonur reviewed Mar 23, 2023

View reviewed changes

aaronbuchwald reviewed Mar 23, 2023

View reviewed changes

richardpringle force-pushed the ci-avago-commitish branch 2 times, most recently from cadfbba to ecac32d Compare March 23, 2023 21:41

richardpringle marked this pull request as ready for review March 23, 2023 21:59

richardpringle requested review from aaronbuchwald and ceyonur March 23, 2023 22:00

aaronbuchwald reviewed Mar 24, 2023

View reviewed changes

scripts/install_avalanchego_release.sh Outdated Show resolved Hide resolved

aaronbuchwald reviewed Mar 24, 2023

View reviewed changes

scripts/install_avalanchego_release.sh Show resolved Hide resolved

aaronbuchwald reviewed Mar 24, 2023

View reviewed changes

richardpringle force-pushed the ci-avago-commitish branch from c55d95d to 459a6b1 Compare March 24, 2023 06:14

richardpringle requested a review from aaronbuchwald March 24, 2023 06:14

aaronbuchwald reviewed Mar 24, 2023

View reviewed changes

scripts/install_avalanchego_release.sh Outdated Show resolved Hide resolved

aaronbuchwald assigned richardpringle Mar 24, 2023

richardpringle force-pushed the ci-avago-commitish branch from 459a6b1 to 99f8e3b Compare March 24, 2023 14:16

Add ability to specify commit of avalanchego in ci

0a5920e

richardpringle force-pushed the ci-avago-commitish branch from 99f8e3b to 0a5920e Compare March 24, 2023 14:44

aaronbuchwald approved these changes Mar 24, 2023

View reviewed changes

aaronbuchwald added the ci label Mar 24, 2023

darioush reviewed Mar 24, 2023

View reviewed changes

richardpringle requested a review from darioush March 24, 2023 15:57

darioush approved these changes Mar 24, 2023

View reviewed changes

anusha-ctrl approved these changes Mar 24, 2023

View reviewed changes

aaronbuchwald merged commit 21c1fb1 into master Mar 24, 2023

aaronbuchwald deleted the ci-avago-commitish branch March 24, 2023 18:04

		mkdir -p $AVALANCHEGO_BUILD_PATH
		mkdir -p $VERSIONED_AVALANCHEGO_BUILD_PATH

		AVALANCHE_VERSION=${AVALANCHE_VERSION:-'v1.9.11'}
		AVALANCHEGO_VERSION=${AVALANCHEGO_VERSION:-$AVALANCHE_VERSION}

		# use the commit hash instead of the branch name or tag
		BUILD_DIR=${AVALANCHEGO_BUILD_PATH}-${COMMIT}

Add ability to specify commit of avalanchego in ci #589

Add ability to specify commit of avalanchego in ci #589

Conversation

richardpringle commented Mar 22, 2023 • edited Loading

Why this should be merged

How this works

How this was tested

How is this documented

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronbuchwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle commented Mar 22, 2023 •

edited

Loading

richardpringle Mar 23, 2023 •

edited

Loading

richardpringle Mar 23, 2023 •

edited

Loading