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

fix(init): example contracts: keep cdylib, exclude Cargo.lock #1337

Merged
merged 4 commits into from
May 27, 2024

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented May 23, 2024

This removes the cargo_toml dependency that I just added in #1296, opting for a simpler regex-based find-and-replace for the soroban-sdk dependency changes. Previous to #1296, we used toml_edit for all of these modifications, but did so in a way that ignored and overwrote important fields like soroban-sdk.features.

The toml_edit approach for editing the dependencies and dev_dependencies sections was quite verbose, and insufficient to what we needed, as the tests added in #1296 and this PR illustrate. Since we know what is in all the soroban-examples projects (for now), I thought maybe the regex approach was sufficient, for the time being. This could prove to be too brittle in the future, though, if one of the examples changes its format to not put all soroban-sdk settings on the same line.

The cargo_toml approach seemed like it should have been superior to both of these approaches, but it was changing more than it should have. For example, here's the starting Cargo.toml for the auth contract, prior to being modified by init:

[package] 
name = "soroban-auth-contract"
version = "0.0.0" 
edition = "2021" 
publish = false

[lib] 
crate-type = ["cdylib"]

[dependencies] 
soroban-sdk = { version = "20.3.1" }

[dev-dependencies] 
soroban-sdk = { version = "20.3.1", features = ["testutils"] }

With cargo_toml, the above was being changed to this:

[package] 
name = "soroban-auth-contract" 
edition = "2021" 
version = "0.0.0" 
publish = false
[dependencies.soroban-sdk] 
workspace = true
[dev-dependencies.soroban-sdk] 
features = ["testutils"] 
workspace = true

[lib] 
path = "src/lib.rs" 
name = "soroban_auth_contract" 
edition = "2021" 
crate-type = ["rlib"] 
required-features = []

Yikes! Where'd all that lib stuff come from?? That's not where edition goes! It already includes edition in the [package] section, why did it add it there, too?? And why on EARTH would it change lib.crate-type = ["cdylib"] to lib.crate-type = ["rlib"]?? Bad news! This means such crates won't even be considered Soroban contracts anymore! Wowee.

This removes the `cargo_toml` dependency, opting for a simpler
regex-based find-and-replace for the soroban-sdk dependency changes.

For some reason the cargo_toml approach was changing more than it should
have. It was changing things like this, the starting `Cargo.toml` for
the `auth` contract:

```toml
[package]
name = "soroban-auth-contract"
version = "0.0.0"
edition = "2021"
publish = false

[lib]
crate-type = ["cdylib"]

[dependencies]
soroban-sdk = { version = "20.3.1" }

[dev-dependencies]
soroban-sdk = { version = "20.3.1", features = ["testutils"] }
```

...to this:

```toml
[package]
name = "soroban-auth-contract"
edition = "2021"
version = "0.0.0"
publish = false
[dependencies.soroban-sdk]
workspace = true
[dev-dependencies.soroban-sdk]
features = ["testutils"]
workspace = true

[lib]
path = "src/lib.rs"
name = "soroban_auth_contract"
edition = "2021"
crate-type = ["rlib"]
required-features = []
```

Yikes! Where'd all that `lib` stuff come from?? That's not where
`edition` goes! It already includes `edition` in the `[package]`
section, why did it add it there, too?? And why on EARTH would it change
`lib.crate-type = ["cdylib"]` to `lib.crate-type = ["rlib"]`?? Bad news!
This means such crates won't even be considered Soroban contracts
anymore! Wowee.
@willemneal willemneal enabled auto-merge (squash) May 24, 2024 03:00
@willemneal willemneal merged commit bc3031e into stellar:main May 27, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants