build: Update for Mac M1 support #980
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #980 +/- ##
========================================
Coverage 60.15% 60.15%
========================================
Files 41 41
Lines 5702 5702
========================================
Hits 3430 3430
Misses 1825 1825
Partials 447 447 Continue to review full report at Codecov.
|
|
Requires algorand/go-algorand#3920 since we depend on some go-algorand pkgs in indexer as well. |
| @@ -22,7 +21,7 @@ func TestPrintableUTF8OrEmpty(t *testing.T) { | |||
| }, | |||
| { | |||
| "Asset 510285544", | |||
| "8J+qmSBNb25leQ==", | |||
There was a problem hiding this comment.
Why did this need to change?
There was a problem hiding this comment.
Not sure of the exact root cause, but from what I can tell, that string is 🪙 Money, want which returns a printable string (failing the test which expects it not to be printable) while the new string 🪙 Money, want\n includes newline and is not printable (as anticipated) on the newline character.
It looks like the most recent update to the unicode bits in go is golang/go@8ec5a05 which started in 1.16. That could be the root cause if we were expecting the moon symbol to fail since we're on 1.13. But I haven't dug that deep on it.
There was a problem hiding this comment.
This also reminded me that I forgot to update golang in this PR. Updating with that.
There was a problem hiding this comment.
#943 also includes changes to this test so it must be failing on golang1.16 as well.
Eric-Warehime
force-pushed
the
m1-support
branch
from
c21522a to
7aa758a
Compare
|
It looks like |
|
@Eric-Warehime I believe go-algorand is using 1.16 specific functionality. We have to disable the 1.14.7 / 1.15.15 tests. |
| } | ||
| resourcesForAddress[creatable] = normalizeAccountResource(appResource) | ||
| default: | ||
| err1 = fmt.Errorf("checkModifiedState() unexpected creatable type %v", creatable.Type) |
There was a problem hiding this comment.
@tolikzinovyev could you review this section? It seems reasonable, but should we call normalizeAccountResource(ar) for the AssetCreatable case?
There was a problem hiding this comment.
I would only suggest to do one of the following:
- Keep
normalizeAccountResource()unchanged and call it after theswitch. - Change
normalizeAccountResource()intonormalizeAppResource()that would accept and returnledgercore.AppResource.
There was a problem hiding this comment.
Went with changing normalizeAccountResource to convertAppResource which will return an AccountResource with normalized fields. I think it's more properly named and avoids calling the function when it's not necessary for AssetResources.
There was a problem hiding this comment.
@tolikzinovyev Let me know if you have any feedback on these changes. 😄
Eric-Warehime
changed the title
|
|
Summary
Adds M1 support
Test Plan
Tests run locally on my M1 machine.
Will need to update CI to include M1 arm64 test support.