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

Hello World #44

Merged
merged 2 commits into from
Apr 15, 2022
Merged

Hello World #44

merged 2 commits into from
Apr 15, 2022

Conversation

tbidne
Copy link
Contributor

@tbidne tbidne commented Apr 1, 2022

This is an initial PR for the hello-world application. There is one issue that needs to be resolved:

You will notice, in the cabal-haskell.nix.project, that I have exchanged the official plutarch for my fork:

source-repository-package
  type: git
  location: https://github.com/tbidne/plutarch
  tag: 7a6c878b7c065c3304676c808492ab076b1a71e8

It has an extremely trivial change: https://github.com/tbidne/plutarch/commit/7a6c878b7c065c3304676c808492ab076b1a71e8. This is unfortunately necessary, as I had to upgrade plutus in order to use some new dependencies -- e.g. plutus-ledger-constraints -- and plutus replaced that module. While this branch of dUSD with my fork builds just fine, my attempts at building plutarch itself fails, likely because I'm missing some steps in that process. I presume we will want plutarch to be upgraded so that we are not on my fork in dUSD. Unfortunately that trivial change is not backwards-compatible, so we should have someone familiar with plutarch weigh in. @srid?

@CSVdB CSVdB requested review from MatthewCroughan and srid April 1, 2022 08:12
Copy link
Contributor

@MatthewCroughan MatthewCroughan left a comment

Choose a reason for hiding this comment

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

If all this PR does is add dependencies, without any actual hello-world code, then the title of the PR should reflect that. I think it could possibly be bad practice to prematurely add deps for code we haven't wrote yet, though. This is effectively experimenting, and expecting experimentation to be merged, rather than actual work being merged, so it gives a false impression, and is impossible to audit or criticise.

For example, suppose I add 500 dependencies right now, and pretend I need them for some theoretical future piece of code I'm writing. Then I find out I only really needed 10 of those dependencies. This would not be good practice.

If this PR is going to be the full PR for the hello-world application, code and all, then it should be converted to a draft.

@srid
Copy link
Contributor

srid commented Apr 1, 2022

@tbidne We should use the staging branch of Plutarch; your change is already in that branch.

DanaSwap-Plutarch also uses staging if you want to use some of its dependency overrides.

@Geometer1729
Copy link
Contributor

If I understand correctly that updating Plutarch to the latest commit on staging solves the problem this is trying to solve, #45 solves this.

@tbidne
Copy link
Contributor Author

tbidne commented Apr 4, 2022

If I understand correctly that updating Plutarch to the latest commit on staging solves the problem this is trying to solve, #45 solves this.

@Geometer1729 Unfortunately it's more complicated than that. plutus-apps, which we presumably want (cc @CSVdB), is quite a bit behind plutus. I'm copying this comment I added to the cabal-haskell.nix.project:

-- Note: The plutus dependency is limited by plutus-apps and plutarch.
-- plutus-apps is the primary bottleneck here. In particular, the latest
-- plutus-apps still relies on the plutus module Plutus.V1.Ledger.TxId that
-- was removed in 4fd86930f1dc628a816adf5f5d854b3fec578312.

So for now, if we want plutus-apps, we can't use the latest plutarch (and plutus itself is decently old; the latest compatible commit I could find is from 28 Jan). Fortunately, I didn't have to downgrade plutarch.

@tbidne tbidne marked this pull request as draft April 4, 2022 06:11
@tbidne
Copy link
Contributor Author

tbidne commented Apr 4, 2022

Converted to draft for now because:

  1. I agree with Matt; handling dependencies is highly non-trivial in iohk-land, so we want to be sure we need something before going through the motions.
  2. There are some questions regarding the PAB itself (hence the need for plutus-apps). Nick is going to have a meeting about this in the next couple days.

@srid
Copy link
Contributor

srid commented Apr 4, 2022

@tbidne I'd also add to that list that we need to figure out what to do about the overlap between this PR and #45. The latter is close to being merge-ready, so I'd suggest this one be built on top of that. Given #45, what would now become the scope of this PR? Is it about adding the plutus-apps dependency for upcoming work?

@CSVdB
Copy link
Contributor

CSVdB commented Apr 4, 2022

@tbidne I'd also add to that list that we need to figure out what to do about the overlap between this PR and #45. The latter is close to being merge-ready, so I'd suggest this one be built on top of that. Given #45, what would now become the scope of this PR? Is it about adding the plutus-apps dependency for upcoming work?

@srid As I understand it, this is not only a matter of adding the plutus-apps dependency, but we also need to figure out how bleeding edge we can go in depending on plutus itself since plutus-apps and plutarch must use the same plutus version.

@tbidne
Copy link
Contributor Author

tbidne commented Apr 5, 2022

Yes to both @srid and @CSVdB. The intention of this PR is to add a minimal contract (though the contract itself is not up), though of course #45 appears to do that as well.

My belief was that we are going to want involve the PAB, hence plutus-apps, and my trying to find compatible hashes for each project (particularly plutarch, plutus, and plutus-apps). If we do want the PAB, then the upgrade to plutarch in #45 is too recent. The staging branch of plutarch requires a version of plutus that is not compatible with plutus-apps, unfortunately.

That said, we are not currently using the PAB, so maybe this concern is premature. I was basing the minimal contract on Dan's here, though that doesn't appear to use the PAB either. It's possible that by the time we do end up using the PAB (if we decide to go that route), then plutus-apps will have been updated.

So to sum up, we cannot currently add plutus-apps on top of #45 without reverting some of those changes i.e. the plutarch upgrade. But maybe that won't be a problem and isn't something we should worry about now.

@tbidne
Copy link
Contributor Author

tbidne commented Apr 5, 2022

Irrespective of the dependency concerns, there still seems like overlap here we (i.e. this PR) should avoid. Could someone more familiar with the overall test plan (@CSVdB?) describe the intended relationship between this PR and #45?

@tbidne
Copy link
Contributor Author

tbidne commented Apr 7, 2022

@Geometer1729 @srid From talking with Nick, it appears that we are going to try to maintain a consistent set of plutus hashes organization-wide, possibly using Dan's plackage.

Furthermore, it looks like the way forward for this particular problem is to try to get plutus-apps updated to work with more recent versions of plutus. I'll create an issue for this on iohk's repo, and possibly try to do this myself, if no one else is working on it. If that is successful, it may be useful for us to proceed with this hypothetical compatible fork of plutus-apps, until it is upstreamed.

For you all, this means we want a plutus hash that is an acceptable target for plutus-apps. #45 has 983e6af2154c4bdf86ed645062bcb62f304d0a4f. Is that what we want? Keep in mind we might be on this hash for a while, since upgrading dependencies will move more slowly.

It's also possible that iohk has a different plutus hash in mind for the next plutus-apps release. We can't do anything about that now, but it's worth remembering.

Thoughts?

@tbidne
Copy link
Contributor Author

tbidne commented Apr 7, 2022

Update: plutus-apps just made a new release: https://github.com/input-output-hk/plutus-apps/releases/tag/v2022-04-06

This pins plutus to 4127e9cd6e889824d724c30eae55033cb50cbf3e from March 7th. So unless there is a specific reason to need a more recent commit, targeting compatibility with 4127e9cd6e889824d724c30eae55033cb50cbf3e seems like a reasonable idea to me.

@srid
Copy link
Contributor

srid commented Apr 7, 2022

Plutarch doesn't compile with plutus:4127e9cd6e889824d724c30eae55033cb50cbf3e

image

@tbidne
Copy link
Contributor Author

tbidne commented Apr 8, 2022

@srid You're right that it doesn't, but it does not appear to be too hard to fix. I've been working on this to see what it would look like.

Incidentally, do you know why plutarch is on L-as's plutus fork instead of iohk's? The flake.nix says:

# https://github.com/input-output-hk/plutus/pull/4328
inputs.plutus.url = "github:L-as/plutus?ref=master";

But that PR has since been merged. Maybe it's no longer needed?

@srid
Copy link
Contributor

srid commented Apr 8, 2022

@tbidne Ya, his plutus fork is no longer needed. We are using only upstream in #45.

If you want to downgrade plutus in Plutarch, go ahead. I'd first check with L-as to see why he chose a newer revision of Plutus, and whether he'd be open to downgrading it. If you are forking Plutarch be sure to base off staging branch (not master).

@tbidne
Copy link
Contributor Author

tbidne commented Apr 8, 2022

Got it, thanks @srid. Yes, there's no reason to downgrade plutarch itself, per se. We only have to pin a compatible version in this repository.

@tbidne
Copy link
Contributor Author

tbidne commented Apr 8, 2022

I have a plutarch fork up that is current_staging - too_recent_changes. Happily, the diff is small.

Currently this all compiles fine in this repository. For some reason I had to upgrade apropos to get past some build errors due to warnings. I'm not sure how to disable dependency -Werror on haskell.nix (removing from ghc-options did nothing).

@nixinator
Copy link
Contributor

disable dependency -Werror on haskell.nix

it's been mentioned here input-output-hk/haskell.nix#519

you maybe able to gleam something like that, or get a work around. However, it maybe be a feature that can be easily added but it creates and anti pattern of ignoring errors when building.

I know in gcc warning are just warning, but errors are always show stoppers.

@tbidne
Copy link
Contributor Author

tbidne commented Apr 9, 2022

disable dependency -Werror on haskell.nix

it's been mentioned here input-output-hk/haskell.nix#519

you maybe able to gleam something like that, or get a work around. However, it maybe be a feature that can be easily added but it creates and anti pattern of ignoring errors when building.

I know in gcc warning are just warning, but errors are always show stoppers.

Thanks for the link. Yeah the ideal response is to "fix the warnings", but that's not always possible if you don't own the deps. I'm all for enabling -Wall, -Werror, etc. on my own code, but having it run on deps is not ideal for exactly this reason.

The usual way to deal with this with cabal is to not enable ghc-options in the cabal file and instead enable it in cabal.project. This will run on your own project but not dependencies (and devs have the option to override this for convenience if they want). For example:

package my-package
  ghc-options:
    -Wall -Wcompat -Werror -Widentities
    -Wincomplete-record-updates -Wincomplete-uni-patterns
    -Wmissing-home-modules -Wmissing-export-lists -Wpartial-fields
    -Wredundant-constraints -Wunused-packages

I remember bringing this up before and being told that we can't do this with nix/haskell.nix.

@tbidne tbidne changed the base branch from master to hello-world-ftr April 11, 2022 10:49
@tbidne
Copy link
Contributor Author

tbidne commented Apr 11, 2022

The target branch has been changed from master to a new branch even with master, hello-world-ftr. @CSVdB had this idea so that we can make incremental progress / have discussions on a granular level without having to worry about the higher bar that is merging to master. We can make a PR to master once we're at a less exploratory stage.

@MatthewCroughan
Copy link
Contributor

@tbidne Is there a way to disable warnings for dependencies only, rather than our own code?

@tbidne
Copy link
Contributor Author

tbidne commented Apr 13, 2022

@tbidne Is there a way to disable warnings for dependencies only, rather than our own code?

@MatthewCroughan Normally the cabal.project method I described does this. Haskell.nix may complicate this. Let me see if I can figure more out.

@CSVdB CSVdB marked this pull request as ready for review April 15, 2022 08:14
Copy link
Contributor

@CSVdB CSVdB left a comment

Choose a reason for hiding this comment

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

Since this a PR to a feature branch (hello-world-ftr), this looks good and can be merged, so the Apps team can continue development.

@CSVdB CSVdB merged commit db10a93 into hello-world-ftr Apr 15, 2022
@MatthewCroughan MatthewCroughan deleted the hello-world branch April 17, 2022 21:48
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.

6 participants