clash-lang / clash-protocols

a battery-included library for dataflow protocols
Other
19 stars 7 forks source link

Strongly separate `clash-protocols-base` and `clash-protocols` #118

Closed lmbollen closed 1 month ago

lmbollen commented 1 month ago

clash-protocols-base should only contain code and definitions related to the circuit plugin. This includes the Circuit definition and Protocol typeclass. Furthermore we include instances for types imported from underlying clash libraries such as tuples, Vec and Signal.

t-wallet commented 1 month ago

Unused import in Protocols.Wishbone. Other than that, LGTM.

It would be nice if we could completely hide the clash-protocols-base package from the outside as well, and just re-export everything via clash-protocols. Currently, users have to specify both clash-protocols and clash-protocols-base in their package sources.

DigitalBrains1 commented 1 month ago

Currently, users have to specify both clash-protocols and clash-protocols-base in their package sources.

Why is that? Doesn't the

  reexported-modules:
    Protocols.Plugin

in clash-protocols.cabal mean they only have to specify a dependency on clash-protocols in their Cabal file and get everything they need including the plugin?

lmbollen commented 1 month ago

Currently, users have to specify both clash-protocols and clash-protocols-base in their package sources.

Why is that? Doesn't the

  reexported-modules:
    Protocols.Plugin

in clash-protocols.cabal mean they only have to specify a dependency on clash-protocols in their Cabal file and get everything they need including the plugin?

I think if you build the package from github using e.g.:

source-repository-package
  type: git
  location: https://github.com/clash-lang/clash-protocols.git
  tag: 623ecd9658fa5b15f71b0d86bac6b714b4b86dc4

You also need to add the dependency for clash-protocols-base

DigitalBrains1 commented 1 month ago

Right, I had misunderstood the comment.

DigitalBrains1 commented 1 month ago

I've converted the remaining review feedback to issues: #119 #120