Bump to new scionproto version

[martenwallewein]

Hey guys,

following the idea of this issue to update the scionproto dependency of scion-apps in another branch netsec-ethz/scion-apps#244

we need to update rains to this scionproto version, too. In this PR I updated all the SCION imports and resolved one minor migration issue. Building and testing (as far as it was possible on my machine) went well, so here is a first draft, let's see what the CI says.

Cheers,
Marten

---

This change is 

[sarcasticadmin]

Im was running into this as well.

I think it might be a good idea to leverage go 1.21 to match scion: scionproto/scion#4378

Regardless, building this branch against go 1.20 on linux results in a complete build and basic functionality of the binaries passes for me.

[martenwallewein]

Good point, I upgraded to Go 1.21, so we're equal to scionproto.

[sarcasticadmin]

Latest changes to the branch work well from me with go 1.21 on linux, thanks @martenwallewein

[sarcasticadmin]

@martenwallewein one other thing I noticed when building locally was a big diff after building via make from go fmt, I think the existing CI check isnt failing:

rains/.circleci/config.yml

Line 19 in 7775c2e

make vet

It should probably be go fmt ./... | diff -u /dev/null - or something similar to return non-zero in the case of a diff inside the Makefile

[martenwallewein]

I observed that same behaviour. I think this is because newer Go versions enforce different formatting. I skipped this intentionally in this PR because it touches nearly all files and would therefor hide the actual changes to the code.

I would suggest to split this in two PR's: This one for the go update, and I can do a second one adding your fixes and run go fmt over the code base, so we have the code properly formatted for newer Go versions, but this one will be probably quite big. What do you think @sarcasticadmin?

[sarcasticadmin]

@martenwallewein Absolutely agree with breaking it up into 2 PRs

[martenwallewein]

Thanks! So it would be great if we can complete/merge this PR in the near future :)

[sarcasticadmin]

I'm not a maintainer, but hopefully they'll be able to look at this soon 🙂

[martenwallewein]

Ah I see, then let's wait for further comments. But thanks for all the input so far! :)

[marcfrei]

Thanks a lot for looking into this @martenwallewein and @sarcasticadmin!

Reviewed 12 of 14 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

.circleci/config.yml line 55 at r2 (raw file):

            # It should not matter if we build SCION binaries with different version than the apps
            # as long as there are no major breaks
            scion_commit=5883c725f748 # v0.6.1-0.20220202161514-5883c725f748

Running the following, original sequence of commands in my setup results also in scion_commit=5883c725f748:

scion_mod_version=$(go list -m github.com/scionproto/scion | sed -s 's/.*\s*=>\s*//')
scion_repo=https://${scion_mod_version% *}.git
scion_commit=${scion_mod_version##*[ -]}

Any idea why it is necessary to explicitly assign the commit here?

---

.circleci/config.yml line 97 at r2 (raw file):

       # Use different Go version to build scion-apps
      - run:
          name: Install Go 1.21.0 to build scion-apps

Why not go directly to the most recent version 1.21.3?

[martenwallewein]

Thanks @marcfrei I will look into the comments!

[martenwallewein]

Hey @marcfrei I can not find the button where to directly comment on the reviews (I guess the night was too short, sorry for this) but I guess it's fine to discuss it here:

1. The topology generation and starting of all services won't work with the newest SCION version from the go.mod, because there was a lot of renaming and moving files ongoing in scionproto in the meantime. So the short term solution is to pin the scion version to the latest release which fits the integration test. The long term solution is to upgrade the integration test to the newest SCION version. However, this needs to be done in other repos too (at least scion-apps, scion-pathdiscovery, and maybe others) so it feels to me that it's a good idea to do this in one run for all of the repos. PS: I just saw that the comment in this line scion_commit=5883c725f748 # v0.6.1-0.20220202161514-5883c725f748 is quite confusing, it was just a backup. Hope this clarifies the version pinning.

2. Updated to GO 1.21.3, now we are up to date!

[marcfrei]

I can not find the button where to directly comment on the reviews

Ah sorry, I used Reviewable to comment on the PR. The button is just below your initial message for this PR at the top of #242

In case it's not showing up for you, does the following link work? https://reviewable.io/reviews/netsec-ethz/rains/242

[marcfrei]

so it feels to me that it's a good idea to do this in one run for all of the repos. PS: I just saw that the comment in this line scion_commit=5883c725f748 # v0.6.1-0.20220202161514-5883c725f748 is quite confusing, it was just a backup. Hope this clarifies the version pinning.

I agree, we can stay on commit 5883c725f748 for now. My question then was, why we need to explicitly pin the version on line 56 when the original code

scion_mod_version=$(go list -m github.com/scionproto/scion | sed -s 's/.*\s*=>\s*//')
scion_repo=https://${scion_mod_version% *}.git
scion_commit=${scion_mod_version##*[ -]}

currently results in the same commit 5883c725f748. Is this to future-proof, in case the snippet above will yield a different commit ID?

[martenwallewein]

I can not find the button where to directly comment on the reviews

Ah sorry, I used Reviewable to comment on the PR. The button is just below your initial message for this PR at the top of #242

In case it's not showing up for you, does the following link work? https://reviewable.io/reviews/netsec-ethz/rains/242

Wow, no no it was my fault, I was searching for a reply button in Reviewable but forgot to sign in with Github, so there was no reply button at all. I will follow up on the issues in Reviewable.

[martenwallewein]

Reviewable status: 13 of 14 files reviewed, 2 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

.circleci/config.yml line 55 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Running the following, original sequence of commands in my setup results also in scion_commit=5883c725f748:

scion_mod_version=$(go list -m github.com/scionproto/scion | sed -s 's/.*\s*=>\s*//')
scion_repo=https://${scion_mod_version% *}.git
scion_commit=${scion_mod_version##*[ -]}

Any idea why it is necessary to explicitly assign the commit here?

Following the discussion, I'm not sure if I'm doing something different than the CI script, but when I run these commands I get a different commit than the pinned one:

Code snippet:

➜  rains git:(feature/bump-scionproto) scion_mod_version=$(go list -m github.com/scionproto/scion | sed -s 's/.*\s*=>\s*//')
➜  rains git:(feature/bump-scionproto) scion_repo=https://${scion_mod_version% *}.git
➜  rains git:(feature/bump-scionproto) scion_commit=${scion_mod_version##*[ -]}
➜  rains git:(feature/bump-scionproto) echo $scion_commit
1774cbfccb4c

---

.circleci/config.yml line 97 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Why not go directly to the most recent version 1.21.3?

Done :)

[marcfrei]

Reviewed all commit messages.
Reviewable status: complete! 1 of 1 LGTMs obtained

---

.circleci/config.yml line 55 at r2 (raw file):

Previously, martenwallewein (Marten Gartner) wrote…

Following the discussion, I'm not sure if I'm doing something different than the CI script, but when I run these commands I get a different commit than the pinned one:

Ah sorry, my fault this time -- I still used the old go.mod for the test above...