-
Notifications
You must be signed in to change notification settings - Fork 549
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
CosmWasm -> cosmwasm #251
CosmWasm -> cosmwasm #251
Conversation
Thanks, Jacob! Didn't know that 🙂 |
Neither did I, till builds started failing on Clay :) |
I made a PR on wasmd, too-- that should fix this durably. Here are the exclamation points: https://github.com/faddat/clay/runs/1149458016?check_suite_focus=true |
e2e fails, I think we may need to update go.mod as well. please see logs here https://github.com/tendermint/starport/runs/1149539531?check_suite_focus=true |
@ilgooz I think it's getting that from upstream, so I made PRs on wasmd and go-cosmwasm that make things lowercase. https://github.com/CosmWasm/wasmd/blob/master/go.mod Also, you're right, I should update go.mod too. Thought I had :). |
This problem is mentioned here, too: https://godoc.org/golang.org/x/mod/module https://www.reddit.com/r/golang/comments/c5d75o/unexpected_module_path/ |
@faddat It seems like CosmWasm defined with capital chars in the module name as you referenced https://github.com/CosmWasm/wasmd/blob/master/go.mod#L1. So, keeping it in the same way feels better. I also haven't encouraged any problems with the current way. Wdyt? |
May I know what's the exact issue with Clay? Have you imported it with capital letters and encountered with a problem? |
I think that CosmWasm/wasmd and CosmWasm/go-cosmwasm need to change their module paths so that there aren't capital letters in them otherwise users will intermittently encounter this problem. Seems it is something to do with GOPROXY. Steps to reproduce:make a github repo, iglooz/clay Put a Clay together git clone https://github.com/faddat/starport --branch downstream-pi
cd starport
make
cd build
./starport app github.com/iglooz/clay
cd clay Push your clay to GitHub git branch 1
git checkout 1
git push --set-upstream https://github.com/iglooz/clay 1 This will work because there's no wasm. You'll get a working Pi image and working binary artifacts. Add Wasm git branch 1-wasm
git checkout 1-wasm
../starport add wasm
git commit . -m "add wasm"
git push --set-upstream https://github.com/iglooz/clay 1-wasm This should fail because "something goproxy explode with capital letters" Compile Clay on your mac: make (This should work just fine and dandy) So basically what we've got here is some kind of ugly edge case. For now, we must not merge this PR, as it breaks things by importing wasmd and go-cosmwasm without the capital letters in the import path. Go will see this and get upset because their modules are declared with capital letters: https://github.com/CosmWasm/wasmd/blob/b650fa7613aae599b07a5554aeaea5cfbac3cf11/go.mod#L1 To eliminate the edge case, and need to be merged. Furthermore there could be effects downstream for users of wasmd and cosmwasm with capitalized module paths. TL;DR:
👍 Another possible route: Maybe we can avoid this by trying to use https://docker.io/cosmos/rbuilder to build binaries in CI. It adds some weight and complexity to the build environment and process, however. |
You are correct that CosmWasm does end up with However, after updating a few scripts, we haven't had issues in several months. And I have seen popular go packages with uppercase letters, like: https://github.com/BurntSushi/toml Renaming all these to lower-case again is a bit of a pain for all the internal files, as well as anyone else importing the uppercase name (which is several projects). If this is the only project with problems, I wonder what those are, and if we can just fix those. What is clay and why are you using it? This seems to be the software with issues, right? |
Clay is a block chain that I am generating with Starport to test out the Raspberry Pi and networking features that I've added to Starport. As far as I can tell, Users are going to hit trouble any time they add cosmwasm to their starport app and then use a github action to build their starport app. You are certainly right about the difficulty for downstream users, needing to change all of their imports mentioning cosmwasm. So my thinking and making that pull request was to kind of get to know before there are far more users. The reason I suggested the no capital letters change is that the capital letters introduced an error that felt quite random. How did you guys dodge this in your build system? Maybe I can build the right into the github action. |
I want to see if this is user error with easy workaround, or a fundamental error we need to address. Here is our Circle CI script: https://github.com/CosmWasm/wasmd/blob/master/.circleci/config.yml we are not using github CI, but we didn't have to make any changes there to handle the cases. If this just happens in github CI (and not locally or on circle), maybe best bring up an issue with Github |
I don't want to seem dismissive, and I am glad you guys are working on this. I just get a lot of change requests and want to make sure they are really important before pulling them in. I guess this doesn't affect all the rust work and it is just 2 go repos in the end that would need to change. |
With a little more digging... It seems you already use other packages with upper-case names in go.sum and even one in go.mod. The issue you are most likely having is the rust so which needs to be linked. After you making a docker build? Locally it tends to work, as the path to the go mod import is included in That problem with properly linking the .so file will happen whether or not the path has I really wonder why the other upper-case names are not causing issues |
The problem is in your build script. I dug in and saw you were cross-compiling. A very important detail, especially since: CosmWasm does not support ARM/Rasperry Pi Even if go compiles, you will have errors linking with the rust code. Also the go.mod used in the failing build linked is not correct: |
It's not dismissive at all-- I was pretty sure that this would be a fairly disruptive request because of all of the downstream users. I really appreciate your taking the time to both investigate and explain-- I did not actually realize that I was compiling rust just by including cosmwasm in Clay. I guess for now I will just table this one, I'll close this pool request and the requests on your repositories as well, and later on we can work on getting this rolling on *Pi devices. |
You are not, you are dynamically linking a pre-built rust dylib into Go. I have tried to get cross-compiling working for arm64 and windows before, but gave up on both of them (alpine was tricky enough - muslc and static libs). You can check out https://github.com/CosmWasm/go-cosmwasm if you want to dig into the bindings. |
We are waiting for the 1.0 wasmer release. When that is out and we upgrade, I would love to see if you can help bring it to PIs. 😄 |
Closing this |
Capital letters in import paths turning to exclamation points in some situations
changelog update
Updated changelog.