fix(ping/rust): Don't set PR ref for master build #79
Conversation
When building the `master` image we would previously mistakingly set the ref to the PR commit hash. This surfaced when testing a pull request from a fork. `master` would be pointed at the forks HEAD commit hash, but not the fork URL. Thus the build could not find the commit and would fail. See libp2p/rust-libp2p#3154
|
This only affects the Rust CI, not the Golang CI. To unblock libp2p/rust-libp2p#3154 and thus unblock the rust-libp2p v0.50.0 release, I will merge here. @jxs @thomaseizinger please still review the change. |
There was a problem hiding this comment.
I would have expected the GitHub workflow to pass the correct revision instead of us setting master here.
pull_request should be undefined here when we push to master, thus I would have expected github_sha to be used which should be the new commit pushed to master.
Reading libp2p/rust-libp2p#3154 in more detail, the problem is not testing on master but testing against master. As long as cargo accepts what you are setting there, I am okay. It slightly harms reproducibility. It would be better if the GitHub action would pass two refs on:
Same for GIT_TARGET probably. |
| @@ -11,7 +11,7 @@ ARG GIT_TARGET="" | |||
| RUN if [ ! -z "${GIT_TARGET}" ]; then sed -i "s,^git.*,git = \"https://${GIT_TARGET}\"," ./plan/Cargo.toml; fi | |||
|
|
|||
| ARG GIT_REF="" | |||
| RUN if [ ! -z "${GIT_REF}" ]; then sed -i "s/^rev.*/rev = \"${GIT_REF}\"/" ./plan/Cargo.toml; fi | |||
| RUN if [ ! -z "${GIT_REF}" ]; then sed -i "s/^rev.*/branch = \"${GIT_REF}\"/" ./plan/Cargo.toml; fi | |||
There was a problem hiding this comment.
This change breaks the custom build, as that one uses a rev and not a branch 🤦♂️ silly me.
There was a problem hiding this comment.
rev should accept a branch name actually: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories
There was a problem hiding this comment.
I had the same in the back of my head, but the following fails for me:
➜ rust git:(master-ref) ✗ cargo check
Updating git repository `https://github.com/libp2p/rust-libp2p`
error: failed to get `libp2p` as a dependency of package `testplan v0.1.0 (/home/mxinden/code/github.com/libp2p/test-plans/ping/rust)`
Caused by:
failed to load source for dependency `libp2p`
Caused by:
Unable to update https://github.com/libp2p/rust-libp2p?rev=master
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
You might need to prefix with refs/:
refs/remotes/origin/masterrefs/heads/master
There was a problem hiding this comment.
revshould accept a branch name actually: doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories
This is talking about named references being supported so I think one of those two should work.
There was a problem hiding this comment.
Wonderful. Thanks @thomaseizinger. refs/heads/master resolves to the current master commit (libp2p/rust-libp2p@babacc5). For some reason refs/removes/origin/master points to libp2p/rust-libp2p@b99a213, a previous commit on master.
I created #81 adopting the former. Please take a look.
Follows suggestion in libp2p#79 (comment). Follow-up to libp2p#79 and libp2p#80.
* fix(ping/rust): Use refs/heads/master and not branch and master Follows suggestion in #79 (comment).
When building the
masterimage we would previously mistakingly set the ref to the PR commit hash.This surfaced when testing a pull request from a fork.
masterwould be pointed at the forks HEAD commit hash, but not the fork URL. Thus the build could not find the commit and would fail. See libp2p/rust-libp2p#3154I would consider this a hotfix. Long term it would be great to be able to support pull requests that target other branches than
master.What do folks think? Does the above make sense?