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

Refine executable dependencies #213

Merged
merged 4 commits into from
Dec 22, 2021
Merged

Conversation

kk-hainq
Copy link
Contributor

@kk-hainq kk-hainq commented Dec 21, 2021

Complicating our Cabal setup to build executable dependencies is not nice. This PR pulls cardano-cli, cardano-node, and cardano-wallet to the Nix shell with a few words here and there directing people to the original repositories to find their best installation option. When people do want to build the executables from the source, they should do it in their respective clones, not changing commit hashes and doing it here.

The net effects of this PR are:

  • cardano-cli, cardano-node, cardano-wallet available in nix-shell by default.
  • cardano-node, cardano-cli, cardano-config, and an optparse-applicative fork removed from our cabal.project.
  • A thinner development environment (+95 -2194 yay!).

The second commit is only updateMaterialized, so reviewing the others should be sufficient.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, materialized Nix files, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@raduom
Copy link
Contributor

raduom commented Dec 21, 2021

I tried to have nix provide the cardano-node and cardano-cli at some point. The problem was that inside the plutus nix-shell the version of the plutus (for example) dependency was not the same as the one from the cardano-node repository. So you get a pretty unpredictable build of cardano-node where some of its dependencies are at different version and as such, not tested. This is why the current advice is to build or install the node separately.

I think that removing them from cabal is a very good idea, but adding them to nix is going to create problems for us down the road (which in fact, it already has).

@kk-hainq
Copy link
Contributor Author

kk-hainq commented Dec 21, 2021

@raduom I indeed built on your previous work and saw the removal commit. That is why I was still very vague with the cardano-node instructions. I don't know what the correct way is...

Overall, I don't think adding cardano-node to the Nix shell is the problem. People can choose not to use them, or easily build even less stable executables themselves. We should either document a stable way or just automate that process. I wonder if we could open an issue to track that, then merge this earlier? I'd be happy to remove cardano-node and cardano-cli from nix-shell for now if required.

For the issue, do you have notes on previously found problems? If plutus-apps should follow cardano-node then we should enforce that with tests and simply use prebuilt nodes?

@kk-hainq kk-hainq marked this pull request as draft December 21, 2021 18:15
@kk-hainq kk-hainq marked this pull request as ready for review December 21, 2021 21:13
@raduom
Copy link
Contributor

raduom commented Dec 22, 2021

I've tested this PR and the node builds without linking to the plutus-apps dependencies.

Copy link
Contributor

@raduom raduom left a comment

Choose a reason for hiding this comment

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

Nice work!

plutus-pab/test-node/testnet/pab-config.yml Outdated Show resolved Hide resolved
Co-authored-by: Radu Ometita <radu.ometita@gmail.com>
@kk-hainq
Copy link
Contributor Author

Thank you @raduom for your insights and help on Slack!

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.

None yet

2 participants