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

chore(make): disable cgo, reorganise makefiles #1715

Merged
merged 2 commits into from
Mar 7, 2024
Merged

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Mar 1, 2024

This PR sets CGO_ENABLED=0 in all top-level makefiles. This comes as a consequence of a rare occurrence where bft is actually failing for a valid reason. This is the relevant part of the logs:

2024-03-01T12:36:50.7818357Z ok  	github.com/gnolang/gno/tm2/pkg/bft/rpc/lib/types	0.004s	coverage: 33.3% of statements
2024-03-01T12:36:50.7853384Z 	github.com/gnolang/gno/tm2/pkg/bft/rpc/test		coverage: 0.0% of statements
2024-03-01T12:36:51.5524659Z # github.com/jmhodges/levigo
2024-03-01T12:36:51.5556601Z ##[error]../../../../go/pkg/mod/github.com/jmhodges/levigo@v1.0.0/batch.go:4:11: fatal error: leveldb/c.h: No such file or directory
2024-03-01T12:36:51.5567246Z     4 | // #include "leveldb/c.h"
2024-03-01T12:36:51.5567886Z       |           ^~~~~~~~~~~~~
2024-03-01T12:36:51.5568424Z compilation terminated.
2024-03-01T12:36:59.3179828Z FAIL	github.com/gnolang/gno/tm2/pkg/bft/state [build failed]

This is likely a consequence of #1602. I've additionally made it so that bft/state doesn't require any database it doesn't need in its tests, but personally I have been repeatedly bitten by finding out CGO was enabled and compiling in unexpected places (including the Go stdlib) that I think it makes sense to keep it disabled by default in our makefiles.

This PR tackles that, and takes the chance to re-organise the overall structure of makefiles to document prominently the available variables that can be changed.

@thehowl thehowl self-assigned this Mar 1, 2024
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Mar 1, 2024
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.72%. Comparing base (6c5b4cf) to head (28c125f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1715      +/-   ##
==========================================
- Coverage   47.41%   44.72%   -2.70%     
==========================================
  Files         384      456      +72     
  Lines       61240    67534    +6294     
==========================================
+ Hits        29040    30202    +1162     
- Misses      29771    34806    +5035     
- Partials     2429     2526      +97     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks great 💯

Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

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

I generally don't like adding export to a Makefile because they can bring some side effects (In this case, setting CGO_ENABLED can also affect go run for example)
But I don't see any other convenient way to do this for now.

@thehowl
Copy link
Member Author

thehowl commented Mar 7, 2024

@gfanton I don't see that as a negative :) (I dislike CGO so much that CGO_ENABLED=0 is an export in my ~/.profile :P)

@thehowl thehowl merged commit d8277e4 into master Mar 7, 2024
184 checks passed
@thehowl thehowl deleted the dev/morgan/no-cgo branch March 7, 2024 15:54
albttx pushed a commit to albttx/gno that referenced this pull request Mar 15, 2024
This PR sets `CGO_ENABLED=0` in all top-level makefiles. This comes as a
consequence of a rare occurrence where [bft is actually failing for a
valid
reason](https://github.com/gnolang/gno/actions/runs/8111419409/job/22170651604).
This is the relevant part of the logs:

```
2024-03-01T12:36:50.7818357Z ok  	github.com/gnolang/gno/tm2/pkg/bft/rpc/lib/types	0.004s	coverage: 33.3% of statements
2024-03-01T12:36:50.7853384Z 	github.com/gnolang/gno/tm2/pkg/bft/rpc/test		coverage: 0.0% of statements
2024-03-01T12:36:51.5524659Z # github.com/jmhodges/levigo
2024-03-01T12:36:51.5556601Z ##[error]../../../../go/pkg/mod/github.com/jmhodges/levigo@v1.0.0/batch.go:4:11: fatal error: leveldb/c.h: No such file or directory
2024-03-01T12:36:51.5567246Z     4 | // #include "leveldb/c.h"
2024-03-01T12:36:51.5567886Z       |           ^~~~~~~~~~~~~
2024-03-01T12:36:51.5568424Z compilation terminated.
2024-03-01T12:36:59.3179828Z FAIL	github.com/gnolang/gno/tm2/pkg/bft/state [build failed]
```

This is likely a consequence of
gnolang#1602. I've additionally made it so
that `bft/state` doesn't require any database it doesn't need in its
tests, but personally I have been repeatedly bitten by finding out CGO
was enabled and compiling in unexpected places (including the Go stdlib)
that I think it makes sense to keep it disabled by default in our
makefiles.

This PR tackles that, and takes the chance to re-organise the overall
structure of makefiles to document prominently the available variables
that can be changed.
@ltzmaxwell
Copy link
Contributor

is /gno/Makefile included? I encountered a similar problem that led to the failure of the golangci-lint installation on my Mac with M2 chip(not sure it's related).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants