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

Add more smoke-tests packages up to cardano-node #160

Merged
merged 40 commits into from
Mar 21, 2023

Conversation

michaelpj
Copy link
Contributor

This extends the smoke-test package builder to build the newest versions of:

  • plutus-ledger-api
  • cardano-ledger-api
  • ouroboros-network
  • ouroboros-consensus-cardano
  • cardano-api
  • cardano-node

To make all this work I had to do a lot of revisions. I took the hard but hopefully thorough route of just keeping trying to build things and then revising whatever the problem with the particular package that failed to build was. That meant a lot of"add an upper bound to foo-X -> cabal picks foo-Y -> add an upper bound to foo-Y -> ..., but I got there eventually.

The revisions are obviously very hard to review at the moment, so I guess you'll just have to trust me. If anyone wants to spot-check, I tried to list in the commit message what the revision was.

There is only one bit of global config I added: allow-newer: ekg:json. cardano-node needs it, upstream seems dead, we should probably just patch it in CHaP so we can bin that.

Note that this means that in future the CI will block new releases of these packages unless they build cleanly!

@michaelpj michaelpj requested review from a team as code owners March 20, 2023 09:15
@michaelpj michaelpj requested review from andreabedini and removed request for a team March 20, 2023 09:30
@michaelpj
Copy link
Contributor Author

(The build was fast because I've been experimenting by building on nixbuild.net, so it was already cached)

Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

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

LGTM

@andreabedini
Copy link
Contributor

andreabedini commented Mar 21, 2023

There is only one bit of global config I added: allow-newer: ekg:json. cardano-node needs it, upstream seems dead, we should probably just patch it in CHaP so we can bin that.

Unfortunately the situation is complex, see my flawed attempt of making a revision for ekg. tl;dr: This is a consequence of us forking ekg-json. We fixed ekg-json but ekg still need relaxing its bounds. Maybe it makes sense to relax the bounds anyway? ekg does work with aeson>=2 as long as ekg-json works with aeson>=2. I might reopen that issue and check what the trustees think about this. Edit: I have opened haskell-infra/hackage-trustees#359

@andreabedini
Copy link
Contributor

@michaelpj I am not sure whether you have already done this or not but I am going to suggest it anyway for the sake of institutional knowledge.

When we make a revision for pkg-a, we need to make sure that our repos where pkg-a is imported keep working. I.e. we could have accidentally put bounds that are too restrictive. This is what happened in #154 and the fix is still WIP (see IntersectMBO/plutus-apps#1023, the typo in freer-extras.cabal is still in CHaP 🤦).

@michaelpj
Copy link
Contributor Author

When we make a revision for pkg-a, we need to make sure that our repos where pkg-a is imported keep working

Right. I guess what I'm hoping is that ultimately this CI check can substitute for that. It does make me realise that I should probably throw some of the plutus-apps packages into the smoke-test too. But yes I guess I'll try and build master of the various projects too...

@michaelpj michaelpj requested a review from a team as a code owner March 21, 2023 14:04
@michaelpj
Copy link
Contributor Author

@andreabedini one data point: I was able to build cardano-node master with this, including after deleting most of the constraints stanza. I'll try and check plutus-apps too.

@michaelpj
Copy link
Contributor Author

Uhhhh. So someone merged something and now I need to update the timestamp on literally everything? 😭

@michaelpj
Copy link
Contributor Author

I'm going to merge this as I think it's an improvement and I will iterate from here if there are problems.

@michaelpj michaelpj merged commit 18e2a28 into main Mar 21, 2023
@michaelpj michaelpj deleted the mpj/add-more-smoke-test branch March 21, 2023 17:07
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.

3 participants