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

CI: ensure msgp generator has been run and is clean #3978

Merged
merged 17 commits into from
May 17, 2022

Conversation

cce
Copy link
Contributor

@cce cce commented May 11, 2022

Summary

This should ensure make msgp has been run for changes that impact msgp serialization on CI builds.

It looks like #3829 was merged but missed changes to agreement/msgp_gen.go and gci updates from algorand/msgp#14 were not incorporated into #3919.

Test Plan

Tests should fail in the codgen_verification.sh job for the first build on this PR, then should pass after a commit is pushed that updates the msgp-generated files with the latest on master.

@cce cce added the Bug-Fix label May 11, 2022
algorandskiy
algorandskiy previously approved these changes May 11, 2022
@@ -23,6 +23,8 @@ github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuy
github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0=
github.com/algorand/msgp v1.1.50 h1:Mvsjs5LCE6HsXXbwJXD8ol1Y+c+QMoFNM4j0CY+mFGo=
Copy link
Contributor

Choose a reason for hiding this comment

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

go mod tidy to remove the old version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried and it cleared out all of go.mod! I guess all of these packages are unused. TBH I'm not really sure long-term whether it is a good idea to keep installing CLI tools this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see now that install_buildtools.sh is just grepping go.mod to pull version numbers, so I changed it to a flat text file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that make it harder to create though? Presumably it was created my some go tool automatically, but the versions file will need to be edited by hand. Could it grep the root go.mod instead? I don't really know what this one is, how it was made, or why it might differ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it is much easier to maintain a simple "versions" file now by hand — I would even be OK to just move the tool versions into install_buildtools.sh directly, as it's the only user. The reason it's not in root go.mod is because these tools are not dependencies of go-algorand. (They don't even really need to be implemented in go — they're just CLI tools to install)

if err != nil {
err = msgp.WrapError(err, "struct-from-array", "Sha256Commitment")
err = msgp.WrapError(err, "struct-from-array", "NativeSha512_256Commitment")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Aharonee @algoidan wanted to makes sure this was as expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I know what happened.
04814e1 (#3829)

This is the commit where I've renamed the TxnRoot to TxnCommitments, and its nested fields.
While doing that I've regenerated msgp, but after noticing the fields in TxnCommitments where no longer in alphabetical order, I swapped them.

	TxnCommitments struct {
		_struct struct{} `codec:",omitempty,omitemptyarray"`

		Sha256Commitment crypto.Digest `codec:"txn256"`
		NativeSha512_256Commitment crypto.Digest `codec:"txn"`

to

	TxnCommitments struct {
		_struct struct{} `codec:",omitempty,omitemptyarray"`

		NativeSha512_256Commitment crypto.Digest `codec:"txn"`
		Sha256Commitment crypto.Digest `codec:"txn256"`

This doesn't break any tests of course, and wasn't caught the the codegen_verification script, but when generating msgp after this change the resulting code will be different (it seems to be generated in a sequential manner) even though its logic is identical.

winder
winder previously approved these changes May 11, 2022
@cce cce dismissed stale reviews from winder and algorandskiy via e8bff0b May 11, 2022 20:25
@algorandskiy
Copy link
Contributor

Any idea why it complains about protocol now?

--- a/agreement/msgp_gen_test.go
+++ b/agreement/msgp_gen_test.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/algorand/msgp/msgp"
 
-	"github.com/algorand/go-algorand/protocol"
 	"github.com/algorand/go-algorand/test/partitiontest"
 )

@cce
Copy link
Contributor Author

cce commented May 12, 2022

I am trying to figure it out still, but yes.. something is weird about running make msgp in CircleCI.. even when I ssh into the CircleCI VM and run it manually it works ...

@algorandskiy
Copy link
Contributor

algorandskiy commented May 16, 2022

Look, the msgp's master misses it:

writeImportHeader(
	testbuf,
	"github.com/algorand/msgp/msgp", "github.com/algorand/go-algorand/test/partitiontest", "testing")

@algorandskiy
Copy link
Contributor

algorandskiy commented May 16, 2022

I think goformat that is called right before flushing the generated files was adding protocol import before and does not add it now.

@cce
Copy link
Contributor Author

cce commented May 16, 2022

@algorandskiy see in the latest commit, even after downgrading msgp to the old version the protocol package is not detected. However again when I ssh in and rerun msgp manually I can get it to work (and also locally). There is something strange about the environment in CircleCI. But perhaps the easiest thing to do is to just add the protocol import to the msgp generator.

This reverts commit db418d9.
@cce
Copy link
Contributor Author

cce commented May 17, 2022

Updated msgp to explicitly import missing package: algorand/msgp#17

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM but why did scripts/buildtools/go.mod and go.sum go?

@cce
Copy link
Contributor Author

cce commented May 17, 2022

@algorandskiy we don't have any go code in that directory, so "go mod tidy" stopped working in there (it detected no dependencies) and we are only using go.mod to track a few version numbers used by a single shell script (install_buildtools.sh), so I switched it to use a text file containing those numbers instead.

algorandskiy
algorandskiy previously approved these changes May 17, 2022
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #3978 (5c0a6b5) into master (91095fa) will increase coverage by 0.04%.
The diff coverage is 27.27%.

@@            Coverage Diff             @@
##           master    #3978      +/-   ##
==========================================
+ Coverage   49.77%   49.82%   +0.04%     
==========================================
  Files         409      409              
  Lines       69157    69157              
==========================================
+ Hits        34426    34460      +34     
+ Misses      31011    30983      -28     
+ Partials     3720     3714       -6     
Impacted Files Coverage Δ
crypto/merklearray/msgp_gen.go 38.52% <ø> (ø)
crypto/merklesignature/msgp_gen.go 41.99% <ø> (ø)
daemon/algod/api/spec/v2/msgp_gen.go 20.31% <ø> (ø)
data/account/msgp_gen.go 40.81% <ø> (ø)
data/basics/msgp_gen.go 34.14% <ø> (ø)
data/transactions/msgp_gen.go 40.38% <ø> (ø)
ledger/msgp_gen.go 41.28% <ø> (ø)
agreement/msgp_gen.go 41.08% <27.27%> (ø)
data/bookkeeping/msgp_gen.go 42.60% <27.27%> (ø)
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91095fa...5c0a6b5. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants