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

CosmWasm -> cosmwasm #251

Closed
wants to merge 2 commits into from
Closed

CosmWasm -> cosmwasm #251

wants to merge 2 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Sep 22, 2020

  • Capital letters in import paths turning to exclamation points in some situations

  • changelog update

  • Updated changelog.

@fadeev
Copy link
Contributor

fadeev commented Sep 22, 2020

Thanks, Jacob! Didn't know that 🙂

@faddat
Copy link
Contributor Author

faddat commented Sep 22, 2020

Neither did I, till builds started failing on Clay :)

@faddat
Copy link
Contributor Author

faddat commented Sep 22, 2020

I made a PR on wasmd, too-- that should fix this durably. Here are the exclamation points:

Screen Shot 2020-09-22 at 9 30 48 PM

https://github.com/faddat/clay/runs/1149458016?check_suite_focus=true

@ilgooz
Copy link
Member

ilgooz commented Sep 22, 2020

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

@faddat
Copy link
Contributor Author

faddat commented Sep 22, 2020

@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

CosmWasm/wasmd#270

CosmWasm/wasmvm#138

Also, you're right, I should update go.mod too. Thought I had :).

@faddat
Copy link
Contributor Author

faddat commented Sep 23, 2020

@ilgooz
Copy link
Member

ilgooz commented Sep 23, 2020

@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?

@ilgooz
Copy link
Member

ilgooz commented Sep 23, 2020

May I know what's the exact issue with Clay? Have you imported it with capital letters and encountered with a problem?

@faddat
Copy link
Contributor Author

faddat commented Sep 23, 2020

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/go-cosmwasm/blob/eac7b84fa5fc8751916e9f599e41429f652fcb41/go.mod#L1

https://github.com/CosmWasm/wasmd/blob/b650fa7613aae599b07a5554aeaea5cfbac3cf11/go.mod#L1

To eliminate the edge case,

CosmWasm/wasmd#270

and

CosmWasm/wasmvm#138

need to be merged.

Furthermore there could be effects downstream for users of wasmd and cosmwasm with capitalized module paths.

TL;DR:

  • Either we find a workaround and document it, or our users will intermittently encounter this issue.
  • Why oh why aren't module paths case-insensitive?
  • To prevent problems for their downstream users, wasmd and go-cosmwasm could tag new releases that reflect the lowercaseization.

👍

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.

@ethanfrey
Copy link

You are correct that CosmWasm does end up with ! internally in the gomod paths. And we did have to deal with that in a Dockerfile build script to copy out the go-cosmwasm lib_gocosmwasm.so files.

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?

@faddat
Copy link
Contributor Author

faddat commented Oct 5, 2020

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.

@ethanfrey
Copy link

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

@ethanfrey
Copy link

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.

@ethanfrey
Copy link

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 LD_LIBRARY_PATH. In general if you are building a binary and looking to deploy it, you will want to copy this so into a standard library path

That problem with properly linking the .so file will happen whether or not the path has ! in it.

I really wonder why the other upper-case names are not causing issues

@ethanfrey
Copy link

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: go build -mod=readonly ./cmd/... -> go: updates to go.mod needed, disabled by -mod=readonly

@faddat
Copy link
Contributor Author

faddat commented Oct 5, 2020

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.

@ethanfrey
Copy link

I did not actually realize that I was compiling rust just by including cosmwasm in Clay.

You are not, you are dynamically linking a pre-built rust dylib into Go.
We provide pre-builts for OSX and glibc Linux. We also compile static wasmd inside alpine linux, recompiling the rust lib for alpine.

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. Docker.* and build/* are the build environments, triggered by the CI on merge-to-master

@ethanfrey
Copy link

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. 😄

@faddat
Copy link
Contributor Author

faddat commented Oct 6, 2020

Closing this

@faddat faddat closed this Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants