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 a step for incrementing ledger package versions in the release process #6442

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zliu41
Copy link
Member

@zliu41 zliu41 commented Sep 4, 2024

No description provided.

@zliu41 zliu41 added the No Changelog Required Add this to skip the Changelog Check label Sep 4, 2024
@zliu41 zliu41 requested review from lehins and a team September 4, 2024 18:03
Copy link

@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.

Perfect. Thank you

- Update the package versions of affected packages in the cabal files, if needed.
Ledger packages follow a different versioning policy than Plutus packages, where each package is versioned independently.
Each ledger PR is responsible for incrementing the version of any affected ledger package to the next release version (unless this has already been done by a previous PR).
For a PR bumping Plutus version bounds, if there are no other breaking changes, increment the last component of the ledger package versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "last component" mean? Does it refer to a dependency hierarchy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I think I understand, it's referring to the structure of the version identifier, like the last component of "1.2.3.4" would be "4". Is that right?

I got confused by the discussion here: https://github.com/IntersectMBO/cardano-ledger/pull/4593/files#r1744246536 . Also, from this and from reading cardano-ledger's release guide I understand that all packages should be updated? Waiting on a response on that PR to be sure.

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

I don't quite get it, why do we need to update ledger package versions and submit PRs to ledger at all? Is it if we break something in the ledger and are nice enough to fix it ourselves or is there some other reason?

@ana-pantilie
Copy link
Contributor

ana-pantilie commented Sep 4, 2024

I don't quite get it, why do we need to update ledger package versions and submit PRs to ledger at all? Is it if we break something in the ledger and are nice enough to fix it ourselves or is there some other reason?

@effectfully We had a discussion about this at the meetup as well, and I think it's good to bring it up.

It seems like a long time ago it was agreed we'd follow a "push-based" dependency upgrade system.

I personally don't agree with it, because it leads to problems such as the one we have here: should we use Plutus team resources to learn and navigate the Ledger team's release process?

I don't find that particularly valuable, maybe it would be more valuable for a Plutus engineer to learn about the actual Ledger code and how Plutus is used there. That would, however, inevitably happen even in a "pull-based" system whenever we introduce breaking changes, as it's still the Plutus team's responsibility to help out with those changes.

I do not, however, see the value in having the Plutus engineer do the mechanical (but error-prone, since it's a foreign repository) work of upgrading the Ledger every 2 weeks!

@ana-pantilie
Copy link
Contributor

Also, it's not true that the Ledger is the only (direct) downstream repository! Cardano API directly depends on Plutus: https://github.com/IntersectMBO/cardano-api/blob/00bb673ac6af5792e1de8655fd820802e8e8786e/cardano-api/cardano-api.cabal#L210 . Should we start opening upgrade PRs to their repository as well? I don't think that's viable.

@lehins
Copy link

lehins commented Sep 4, 2024

It seems like a long time ago it was agreed we'd follow a "push-based" dependency upgrade system.

I personally don't agree with it, because it leads to problems such as the one we have here: should we use Plutus team resources to learn and navigate the Ledger team's release process?

I 100% agree with this. I was trying to convince MPJ that it was not a very good idea, but he was adamant about it.

For all other repos it is the job of release engineer to take care of this. That's why we had numerous occasions where PRs that updated Plutus to the same version were opened into ledger repo at the same time by Plutus team members and releases engineer.

Occasionally integration is a bit too complex for a release engineer to handle, in which case maintainers of the repo do take over the task. But in case of plutus as a dependency of ledger, it is usually just a change of bounds. So, yeah, this push based approach makes no sense to me.

it's still the Plutus team's responsibility to help out with those changes.

Almost all occasions of Plutus integration into ledger do not require Plutus team intervention. Sometimes we do have to collaborate and I really enjoy those occasions 🙂, but they do happen very infrequently.

@zliu41
Copy link
Member Author

zliu41 commented Sep 4, 2024

Reasons we've been doing this: (1) it is not difficult, and it needs to be done sooner or later; (2) While a library's authors often don't care whether projects depending on it are always on the latest version of the library, for us I think we do care that this is the case (since it ensures each node release has the latest possible Plutus library versions).

I'm OK to stop doing so, especially if ledger is not the only downstream component. @lehins Can we then add a requirement to bump to the latest Plutus release before making a ledger release?

@lehins
Copy link

lehins commented Sep 4, 2024

Can we then add a requirement to bump to the latest Plutus release before making a ledger release?

Ledger team rarely if ever makes releases. Again, it is the job of release engineer.
Because of this push based approach Plutus team have been using, release engineer normally would just skip releasing and integrating Plutus. I would absolutely recommend you putting this burden on the release engineer instead. In the end it is the dependencies of cardano-node that matters, not bounds on Plutus that are specified in Ledger.

@lehins
Copy link

lehins commented Sep 4, 2024

I think we do care that this is the case (since it ensures each node release has the latest possible Plutus library versions).

In other if you really care about the version of plutus that is being used it must be enforced with specific bounds on cardano-api, since that is the direct dependency of cardano-cli and cardano-node.

I see no point in having strict bounds in ledger packages

@zliu41
Copy link
Member Author

zliu41 commented Sep 4, 2024

I would absolutely recommend you putting this burden on the release engineer instead.

That's what I was trying to say - i.e., let's clearly specify in the ledger release process that "before releasing the ledger, verify that it depends on the most recent Plutus release", so that it becomes an official responsibility of the release engineer, ensuring it isn't overlooked.

@lehins
Copy link

lehins commented Sep 4, 2024

That is also what I am trying to say, is that Ledger has nothing to do with this. This message should be explicitly added to cardano-api!

If there are no versions of ledger packages that are compatible with latest plutus, the cardano-api cannot be released. On the other hand if there is a version of ledger that is compatible with latest plutus, but cardano-api is not specifying the latest Plutus version, then neither of the latest ledger nor plutus will be used!

I've been saying the same thing to MPJ. For some reason everyone totally misses this point!

Pinning specific plutus version in ledger is pointless!

@zliu41
Copy link
Member Author

zliu41 commented Sep 4, 2024

Sure, we can add that sentence to the cardano-api release process as well, but I don't know how it is pointless to also ensure that in the ledger: in order to bump plutus version in cardano-api and still have it compile, the ledger must have bumped the plutus version first.

In other words, a sensible sequence of events would be: (1) plutus is released; (2) ledger is released, after ledger's release engineer verifies that the correct plutus version is used; (3) cardano-api is released, after cardano-api's release engineer verifies that the correct ledger version is used. Hence the suggestion to make it explicit in the ledger release process. (I know we often use s-r-p rather than releases, but replace release with s-r-p above and it still holds).

@lehins
Copy link

lehins commented Sep 5, 2024

in order to bump plutus version in cardano-api and still have it compile, the ledger must have bumped the plutus version first.

You are totally missing my point when I say: "Pinning specific plutus version in ledger is pointless!"

We can specify in ledger plutus-ledger-api >=1.33 and don't touch the bounds until there is a real breaking change that affects ledger code. We don't need to bother with every plutus version bump, because it is extremely rare when those changes break ledger. Only cardano-api needs to be strict in its bounds, if specific version of plutus is desired.

That is how we deal with all of our dependencies, it was always only Plutus that enforced speculative upper bounds on us, which was fine for us because Plutus team was always eager to update bounds themselves. If this job would fall on the ledger team or the release engineer then we should handle it differently.

I'll give you a concrete example of how speculative bounds are bad on a concrete example. Plutus team went out of their way to make the codebase compatible with both major versions of nothunks-1.x and nothunks-2.x:

#if MIN_VERSION_nothunks(0,2,0)
BuiltinCostedResult _ _ -> pure . Just . ThunkInfo $ Left ctx
#else
-- Plutus has moved to nothunks>=0.2, but some other IOE repos are using nothunks<0.2.
-- As a consequence, cardano-constitution:create-json-envelope cannot be build.
-- This is a workaround to make it build (default is buildable:False). See `cabal.project`
BuiltinCostedResult _ _ -> pure . Just $ ThunkInfo ctx
#endif

Only for it to be disabled with restrictive bounds:

, nothunks ^>=0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants