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

Triton and twenty first re exports pr #68

Merged

Conversation

dan-da
Copy link
Contributor

@dan-da dan-da commented Jan 19, 2024

For your consideration.

As I understand it, the dependency tree for triton/neptune crates logically looks like this:

neptune-core
 |- tasm-lib
      |- triton-vm
           |- twenty-first
               |- bfieldcodec_derive

and maybe some more, but those are the one's I'm most interested in right now. But instead here is the actual output of cargo tree, with version info stripped:

$ cargo tree --no-dedupe -f "{lib}" | grep "neptune\|twenty\|triton\|tasm\|bfield"
neptune_core
├── tasm_lib
│   ├── derive_tasm_object
│   ├── triton_vm
│   │   ├── twenty_first
│   │   │   ├── bfieldcodec_derive
│   └── twenty_first
│       ├── bfieldcodec_derive
├── triton_vm
│   ├── twenty_first
│   │   ├── bfieldcodec_derive
├── twenty_first
│   ├── bfieldcodec_derive

This PR is aimed at moving us towards the first dep tree.

This PR does two things, in two separate commits:

  1. re-exports triton_vm and triton_vm::twenty_first in lib.rs like so:
pub use triton_vm;
pub use triton_vm::twenty_first; // less verbosity
  1. changes all use twenty_first:: statements to use crate::twenty_first.

The purpose of [1] is to help out crates that depend on us (eg neptune-core), so they don't have to specify a version of triton_vm or twenty_first, but automatically get our version.

The purpose of [2] is so that we are using the twenty_first from triton_vm and don't need to have a dep of our own.

Unfortunately [2] is complicated by a strange situation with bfieldcodec_derive::BFieldCodec, which hard-codes string references to ::twenty_first in its derive macro, forcing crates that use it to have a dep on twenty-first. So until that is resolved tasm-lib won't compile without a twenty-first dep, and thus I left it in.

As for resolving it, the simplest thing, to my mind, is to move bfieldcodec_derive::BFieldCodec into twenty-first. But then I do not know any history of why it is a separate crate, or why I can't seem to find the repo for it.

[2] is a much bigger change, as it touches a lot of source files. I made this separate commits, so that if [2] is not accepted, hopefully we can still get [1] merged.

my motivation:

Mainly just to cleanup deps, and minimize dependency-hell in accordance with rust best practices.

But also, I have in the past encountered conflicts in neptune-core where this would have helped.


Final thought: this PR is just for triton_vm and twenty_first that I know are used in the public API. There may well be types from other crates exposed in the API. So best practice would be to review the public API for such cases.

Types from these crates are used in our public API, so it is best
practice to export them, so our dependents do not need an explicit
dep, and instead always use our version, thus lessening dependency
conflicts.
This way we are always using twenty_first specified in
crate::twenty_first, which is actually triton_vm::twenty_first.

So we are always in sync with triton_vm, and don't have to maintain
twenty_first in Cargo.toml.

caveat: for now we have to keep a twenty_first dep in Cargo.toml
because of the hard-coded "::twenty_first" in
bfieldcodec_derive::BFieldCodec proc macro.  If/when we get
that resolved, the Cargo.toml dep can be removed.
@Sword-Smith
Copy link
Contributor

bfieldcodec is located here:
https://github.com/Neptune-Crypto/twenty-first/tree/master/bfieldcodec_derive

Maybe this line and its explanation provide some relevant info about the twenty_first hard-coded crate name.
https://github.com/Neptune-Crypto/twenty-first/blob/10afd069f0de8de0d693bca291fb0f06b4961a16/twenty-first/src/lib.rs#L19

@dan-da
Copy link
Contributor Author

dan-da commented Jan 19, 2024

bfieldcodec is located here: https://github.com/Neptune-Crypto/twenty-first/tree/master/bfieldcodec_derive

thx!

Maybe this line and its explanation provide some relevant info about the twenty_first hard-coded crate name. https://github.com/Neptune-Crypto/twenty-first/blob/10afd069f0de8de0d693bca291fb0f06b4961a16/twenty-first/src/lib.rs#L19

yes I read that comment before. It speaks to intent, but not really why a derive macro needs to be hard-coding a crate name. I've never written a derive macro, so I can't speak to what is or isn't possible. It's just that I don't believe I've ever seen behavior like that before in any macro from a public crate, so I feel like there must be another way to do it.

That said, I have an idea for a work-around to achieve my goal without altering the macro much. Basically, I just replaced all ::twenty-first in the macro with crate::twenty-first. Then, so long as the depending crate places a use twenty_first in lib.rs, it should work (I hope). And that could actually be a use triton_vm::twenty_first, for example. So no Cargo.toml dep required. So far the bfieldcodec_derive crate compiles fine with that change, but I haven't tried to build any dependent crate yet. getting late here, will try in the am.

dan-da added a commit to dan-da/twenty-first that referenced this pull request Jan 20, 2024
Updates the BFieldCodec macro to be friendly to crates that use
twenty-first via re-export.

previously:
  ::twenty-first::<path>

now:
  crate::twenty-first::path

So a dependent crate needs to do this in lib.rs:
  use twenty_first;

or if using twenty-first via re-export in `dep_crate`:
  use dep_crate::twenty_first;

See writeup at:
 TritonVM/tasm-lib#68
dan-da added a commit to dan-da/twenty-first that referenced this pull request Jan 20, 2024
Updates the BFieldCodec macro to be friendly to crates that use
twenty-first via re-export.

previously:
  ::twenty-first::<path>

now:
  crate::twenty-first::path

So a dependent crate needs to do this in lib.rs:
  use twenty_first;

or if using twenty-first via re-export in `dep_crate`:
  use dep_crate::twenty_first;

See writeup at:
 TritonVM/tasm-lib#68
@dan-da
Copy link
Contributor Author

dan-da commented Jan 20, 2024

yeah, so as commented in Neptune-Crypto/twenty-first#180, this approach is working, so I think we should go ahead and merge both PRs.

$ cargo tree --no-dedupe -f "{lib}" | grep "triton\|twenty\|neptune\|tasm_lib\|bfield"
tasm_lib
└── triton_vm
    ├── twenty_first
    │   ├── bfieldcodec_derive

But let me know if there are concerns. I will wait until Monday if I don't hear anything before.

@Sword-Smith Sword-Smith self-requested a review January 22, 2024 21:24
dan-da added a commit to Neptune-Crypto/twenty-first that referenced this pull request Jan 22, 2024
Updates the BFieldCodec macro to be friendly to crates that use
twenty-first via re-export.

previously:
  ::twenty-first::<path>

now:
  crate::twenty-first::path

So a dependent crate needs to do this in lib.rs:
  use twenty_first;

or if using twenty-first via re-export in `dep_crate`:
  use dep_crate::twenty_first;

See writeup at:
 TritonVM/tasm-lib#68
@Sword-Smith Sword-Smith self-requested a review January 22, 2024 21:51
Copy link
Contributor

@Sword-Smith Sword-Smith left a comment

Choose a reason for hiding this comment

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

Compile errors

Edit: My problems seem to have been resolved. Maybe my Cargo.lock was using Triton-VM 0.36.0 and not 0.36.1? Either way, seems to be working now. Can't explain what the compilation problem was, other than maybe an open buffer in VS Code.

@Sword-Smith Sword-Smith self-requested a review January 22, 2024 21:59
@Sword-Smith Sword-Smith merged commit 2be04ac into TritonVM:master Jan 22, 2024
dan-da added a commit to dan-da/tasm-lib that referenced this pull request Jan 23, 2024
Updates the TasmObject derive macro to be friendly to crates that use
twenty-first via re-export.

previously:
  ::twenty_first::<path>
  ::triton_vm::<path>

now:
  crate::twenty_first::<path>
  crate::triton_vm::<path>

So a dependent crate needs to do this in lib.rs:
  use twenty_first;
  use triton_vm;

or if using via re-export in `dep_crate`:
  use dep_crate::twenty_first;
  use dep_crate::triton_vm;

See writeup at:
  TritonVM#68
dan-da added a commit to dan-da/tasm-lib that referenced this pull request Jan 23, 2024
Updates the TasmObject derive macro to be friendly to crates that use
twenty-first via re-export.

previously:
  ::twenty_first::<path>
  ::triton_vm::<path>

now:
  crate::twenty_first::<path>
  crate::triton_vm::<path>

So a dependent crate needs to do this in lib.rs:
  use twenty_first;
  use triton_vm;

or if using via re-export in `dep_crate`:
  use dep_crate::twenty_first;
  use dep_crate::triton_vm;

See writeup at:
  TritonVM#68
jan-ferdinand pushed a commit that referenced this pull request Jan 25, 2024
Updates the TasmObject derive macro to be friendly to crates that use
twenty-first via re-export.

previously:
  ::twenty_first::<path>
  ::triton_vm::<path>

now:
  crate::twenty_first::<path>
  crate::triton_vm::<path>

So a dependent crate needs to do this in lib.rs:
  use twenty_first;
  use triton_vm;

or if using via re-export in `dep_crate`:
  use dep_crate::twenty_first;
  use dep_crate::triton_vm;

See writeup at:
  #68
jan-ferdinand pushed a commit that referenced this pull request Jan 25, 2024
Updates the TasmObject derive macro to be friendly to crates that use
twenty-first via re-export.

previously:
  ::twenty_first::<path>
  ::triton_vm::<path>

now:
  crate::twenty_first::<path>
  crate::triton_vm::<path>

So a dependent crate needs to do this in lib.rs:
  use twenty_first;
  use triton_vm;

or if using via re-export in `dep_crate`:
  use dep_crate::twenty_first;
  use dep_crate::triton_vm;

See writeup at:
  #68
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.

2 participants