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

Structure as workspace #328

Closed
chadoh opened this issue Oct 8, 2024 · 4 comments
Closed

Structure as workspace #328

chadoh opened this issue Oct 8, 2024 · 4 comments

Comments

@chadoh
Copy link
Contributor

chadoh commented Oct 8, 2024

What problem does your feature solve?

We have started encouraging people to set up projects using workspaces. For example, stellar contract init allows initializing a project with one of these soroban-examples examples, using the --with-example option. But the CLI then needs to tweak the Cargo.toml files of the examples to work as part of a workspace.

What would you like to see?

If this were already structured as a workspace, we wouldn't need any special handling within commands like stellar contract init. In fact, we wouldn't really need stellar contract init at all. A simple degit would work. For example, if you only wanted one example contract, you could do something like this:

degit stellar/soroban-examples/increment contracts/increment

This would then allow us to circle back and simplify stellar contract init as well, perhaps using degit-rs. This fits in with a larger goal of simplifying contract init:

How to implement

It's really nice that the examples here are all in the top level of the repo. To keep that, we could structure this the way that we structured stellar/soroban-test-examples:

[workspace]
members = [
  "./*",
]
exclude = [
  ".git",
  "target",
]
@leighmcculloch
Copy link
Member

This repo actually used to be a large workspace, so I think this is a proposal to return to what we had, and we should look at why we moved away from it, because we may be shuffling different problems and tradeoffs.

This is the PR that moved us away from a workspace in the past:

Iirc the background for why the change was made was because people wanted to play with single examples in isolation, and the workspace was confusing for folks new to Rust, seeing all these projects, and then trying to understand why when they copied out a single contract, it no longer worked because of the references back to the parent toml file.

The move away from a workspace happened before the --with-examples feature was added to the CLI, so while we might be removing --with-examples, the fact the repo had no workspace wasn't connected to that feature.


A simple degit would work. For example, if you only wanted one example contract, you could do something like this:

degit stellar/soroban-examples/increment contracts/increment

I think we could already use degit today without adding the workspace. There would be no worksplace variables in use, but it would still work. In fact it may be better, because the contract would be downloaded and setup and be using the version of the SDK that the example was coded to. If the user's local tree happens to be on a newer or older SDK for the other contracts, that's actually fine. They probably want to change that, but it's fine, and there maybe incompatibilities between the two.


Having seen both approaches play out at different times there are challenges with both approaches, and it's unclear to me that a workspace would be a net-improvement.

chadoh added a commit to AhaLabs/soroban-examples that referenced this issue Nov 27, 2024
Fixes stellar#328

Undoes stellar#232

Forked from stellar#330

Add top-level `Cargo.toml`, with all project folders as part of this
workspace.

We now encourage everyone to use Cargo workspaces when
building Soroban projects. Let's make it easy to copy-paste these
examples into their projects, rather than requiring special tooling
(like the old `stellar contract init` behavior) to update the contract's
Cargo.toml.

Currently, there are two compilation errors when attempting to run
tests:

```
error[E0433]: failed to resolve: use of undeclared crate or module `__get_allowance`
  --> token/src/contract.rs:67:12
   |
67 |     pub fn get_allowance(e: Env, from: Address, spender: Address) -> Option<AllowanceValue> {
   |            ^^^^^^^^^^^^^ use of undeclared crate or module `__get_allowance`
   |
help: there is a crate or module with a similar name
   |
67 |     pub fn __allowance(e: Env, from: Address, spender: Address) -> Option<AllowanceValue> {
   |            ~~~~~~~~~~~

error[E0223]: ambiguous associated type
  --> bls_signature/src/lib.rs:78:18
   |
78 |         agg_sig: Self::Signature,
   |                  ^^^^^^^^^^^^^^^ help: use fully-qualified syntax: `<IncrementContractArgs as soroban_sdk::testutils::ed25519::Sign>::Signature`
```
chadoh added a commit to AhaLabs/soroban-examples that referenced this issue Nov 27, 2024
Fixes stellar#328

Undoes stellar#232

Forked from stellar#330

Add top-level `Cargo.toml`, with all project folders as part of this
workspace.

We now encourage everyone to use Cargo workspaces when
building Soroban projects. Let's make it easy to copy-paste these
examples into their projects, rather than requiring special tooling
(like the old `stellar contract init` behavior) to update the contract's
Cargo.toml.

Currently, there are two compilation errors when attempting to run
tests:

```
error[E0433]: failed to resolve: use of undeclared crate or module `__get_allowance`
  --> token/src/contract.rs:67:12
   |
67 |     pub fn get_allowance(e: Env, from: Address, spender: Address) -> Option<AllowanceValue> {
   |            ^^^^^^^^^^^^^ use of undeclared crate or module `__get_allowance`
   |
help: there is a crate or module with a similar name
   |
67 |     pub fn __allowance(e: Env, from: Address, spender: Address) -> Option<AllowanceValue> {
   |            ~~~~~~~~~~~
```
@leighmcculloch
Copy link
Member

leighmcculloch commented Nov 27, 2024

I think we could already use degit today without adding the workspace.

To expand further on this comment I made above, with examples. The following is the experience today without any changes to the soroban-examples repo. In my experience the soroban-examples without a workspace works best with more situations, where folks might be using a workspace or not. See below.

Using degit in a bare directory

$ degit stellar/soroban-examples/increment
cloned stellar/soroban-examples#HEAD

$ tree
.
├── Cargo.lock
├── Cargo.toml
├── Makefile
└── src
    ├── lib.rs
    └── test.rs

2 directories, 5 files

$ stellar contract build
    Finished `release` profile [optimized] target(s) in 10.61s

Using degit in a stellar workspace

$ stellar contract init .
ℹ️ Initializing workspace at "."
➕ Writing "./.gitignore"
➕ Writing "./Cargo.toml"
➕ Writing "./README.md"
ℹ️ Initializing contract at "./contracts/hello-world"
➕ Writing "./contracts/hello-world/Cargo.toml"
➕ Writing "./contracts/hello-world/Makefile"
➕ Writing "./contracts/hello-world/src/lib.rs"
➕ Writing "./contracts/hello-world/src/test.rs"

$ stellar contract build
ℹ️ CARGO_BUILD_RUSTFLAGS=--remap-path-prefix=/Users/leighmcculloch/.cargo/registry/src/= cargo rustc --manifest-path=contracts/hello-world/Cargo.toml --crate-type=cdylib --target=wasm32-unknown-unknown --release
    Finished `release` profile [optimized] target(s) in 23.34s

$ degit stellar/soroban-examples/increment contracts/increment
cloned stellar/soroban-examples#HEAD to contracts/increment

$ stellar contract build
ℹ️ CARGO_BUILD_RUSTFLAGS=--remap-path-prefix=/Users/leighmcculloch/.cargo/registry/src/= cargo rustc --manifest-path=contracts/hello-world/Cargo.toml --crate-type=cdylib --target=wasm32-unknown-unknown --release
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /Users/leighmcculloch/Code/te/test2/contracts/increment/Cargo.toml
workspace: /Users/leighmcculloch/Code/te/test2/Cargo.toml
    Finished `release` profile [optimized] target(s) in 0.16s
ℹ️ CARGO_BUILD_RUSTFLAGS=--remap-path-prefix=/Users/leighmcculloch/.cargo/registry/src/= cargo rustc --manifest-path=contracts/increment/Cargo.toml --crate-type=cdylib --target=wasm32-unknown-unknown --release
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /Users/leighmcculloch/Code/te/test2/contracts/increment/Cargo.toml
workspace: /Users/leighmcculloch/Code/te/test2/Cargo.toml
    Finished `release` profile [optimized] target(s) in 0.15s

There is a non-root profile warning, but that's not harmful, and developers can tweak their toml files to remove the duplicate profile in the increment Cargo.toml.

@leighmcculloch
Copy link
Member

If this were already structured as a workspace, we wouldn't need any special handling within commands like stellar contract init.

We shouldn't need any special handling today. Any example contract can be cloned into an existing workspace's contracts/ directory and it'll work as is. It won't use the workspace dependencies, but that's fine and sometimes preferred because examples can at times support different SDK versions to someone's local workspace.

@chadoh
Copy link
Contributor Author

chadoh commented Dec 3, 2024

Thanks, @leighmcculloch! Sorry I missed your comment from October. Glad I didn't get further implementing this in #344.

I find that, in my own workflows, I usually want the copied-in directories to be fully-integrated into the workspace. This means that I need to go through a little list of changes every time:

  • (cd contracts/new-one && rm target Cargo.lock Makefile)
  • update contracts/new-one/Cargo.toml
    • s/sdk = { version = "[^"]*"/sdk = { workspace = true
    • remove profile sections

I don't think these steps are straightforward or easily discoverable, to people new to Rust. But you're right: it's also not straightforward if you copy in an example that uses a different sdk version and it starts failing. And point taken, about workspace members being less portable.

@chadoh chadoh closed this as completed Dec 3, 2024
@leighmcculloch leighmcculloch closed this as not planned Won't fix, can't repro, duplicate, stale Dec 3, 2024
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 a pull request may close this issue.

2 participants