search

stellar / rs-stellar-xdr

Rust lib for Stellar XDR.
Apache License 2.0
20 stars 27 forks source link

Update to reflect removal of SCSpecTypeSet #291

Closed graydon closed 1 year ago

graydon commented 1 year ago

Reflects https://github.com/stellar/stellar-xdr/pull/136 (and will need a refresh after that merges to bump the XDR ref)

2opremio commented 1 year ago

@graydon For future reference, please don't use branch commits (e2a9cbf ) for dependencies (e.g. at https://github.com/stellar/rs-soroban-env/pull/1009 and https://github.com/stellar/soroban-tools/pull/884 ), since it seems that once the branch is removed the commit is garbage collected and can't be used (at least cargo can't find it, see https://github.com/stellar/soroban-tools/actions/runs/6010764494/job/16302863738?pr=884 ). Alternatively, please don't remove the branch.

Is there a way you can restore e2a9cbf72d94941de1bde6ba34a38e1f49328567 ? I have tried using the merged commit but if it isn't used consistently across all repos (including env and sdk) it leads to errors due to multiple versions of the data structures and I would rather not modify 2 repos to make progress.

Alternatively, can you provide me with rs-soroban-env and SDK commits for https://github.com/stellar/soroban-tools/pull/884 depending on a rs-stellar-xdr commit which is alive?

graydon commented 1 year ago

@2opremio Yeah I'm sorry that was a mistake, not intentional. I only noticed that I left the temp commit ID in that change after it merged -- github keeps "temp commits that were part of a PR" around so CI actually passes weirdly enough -- and by the time I was repairing it Dima already had https://github.com/stellar/rs-soroban-env/pull/1015 open moving forward to take the next rs-xdr change https://github.com/stellar/rs-stellar-xdr/commit/936273b737b99d79eee7a28dd4823436d0bd0abb .. I didn't update https://github.com/stellar/soroban-tools/pull/884 directly with that because I didn't want to step on your toes; I don't entirely understand your update process here.

I think the most constructive path forward for you would be to take a newer env rev based on a non-temporary rs-xdr rev. Again I don't know exactly how you decide to take updates; it seems to involve taking a core build as well, in which case I guess I would drive your choice of which rev of env and rs-xdr to take from the core-build choice, since core lags the other two?

2opremio commented 1 year ago

I have been able to make progress after @sisuresh restored the branch.

I also thought GitHub kept commits around but observed it wasn’t the case, at least with this commit, as you can see at https://github.com/stellar/soroban-tools/actions/runs/6010764494/job/16302863738?pr=884

leighmcculloch commented 1 year ago

GitHub keeps commits, but git won't fetch them by default. Unless your git repository previously fetched it, if a commit is not referenced by a branch you'll need to fetch it manually with git fetch origin <commit-id>.

2opremio commented 1 year ago

Well, that breaks cargo as per https://github.com/stellar/soroban-tools/actions/runs/6010764494/job/16302863738?pr=884

2opremio commented 1 year ago

I don't know exactly how you decide to take updates

It is of vital importance for the core team to understand that, in order for platform (soroban-tools) to consume an update we need

graydon commented 1 year ago

Well, I did understand that at a conceptual level, but maybe not quite thoroughly enough in terms of the practice of proposing a partial change to tools as I did. Three things I think went wrong in this cycle:

  1. I opened a tools update PR lacking core update (since I don't know how to do that) and I set the auto-merge flag which I thought would fail until "whatever core integration has to happen gets done" and instead it just went and merged with broken tests. My mistake, I shouldn't have set the auto-merge flag.
  2. I landed the env and tools updates based on the temp commit in rs-xdr which, again, my fault and while I tried to correct it, as mentioned above I felt like correcting it by going backwards was probably more work than going forward to the next update.
  3. Subsequently the "going forward" step has intersected several interleaved workstreams in xdr, core and env (we keep finding bugs, and getting feedback from you that things need to change) and the core team has not yet re-synchronized on a single xdr + rs-xdr + env + sdk + core 5-tuple to allow tools to update. So once more you are blocked waiting for some part of that.

I think the question of "when the core team is obliged to produce such a synchronized 5-tuple" / "when the platform team will expect to have one to accomplish a successful tools update" is worth addressing, and is mostly what I meant above by "I don't know how you decide to take updates". Like maybe you only want them sometimes, so us being out-of-sync for a few days at a time is ok since you only want to do integration runs once a week or so. Or maybe you to do an update every time any one of the 5 repos changes. I don't entirely know.

We're obviously failing to meet expectations currently, as you're having your days wasted hitting roadblocks. I'm sorry about that. A policy around this might make expectations clearer and reduce frustration. I'd be happy to follow any explicit policy, so long as both teams' managment is ok with the impact on schedules (more frequent and wider synchronization means a bit more overhead / slower progress in each repo, but maybe that's ok if it lets the teams stay in closer sync). Earlier in the project I think @leighmcculloch managed more of these handoffs, especially the SDK step you're seeing us lagging on (core doesn't depend on it), and probably it's not so good to rely on him as human shock absorber.

graydon commented 1 year ago

(Another question I feel is pertinent here, and I don't mean to unload work on you by asking, but I think it might be worth discussing: are you ever comfortable / would you like to have rights to push updates to SDK if we've failed to update it, i.e. when it's a revision or two behind the more closely-coupled xdr + rs-xdr + env + core set? I see you've done so in the past. I know part of the problem here is timezones and workdays not overlapping enough between teams, and I think it would probably be fine to expand SDK's committer set from stellar/contract-committers to include stellar/horizon-committers also -- and maybe others, there's quite a wide set of writers on soroban-tools -- since the SDK is really more of a bridge repo between teams and its versioning policy is potentially a little more flexible. I don't know for sure though, @MonsieurNicolas and @leighmcculloch wdyt?)

leighmcculloch commented 1 year ago

in order for platform (soroban-tools) to consume an update we need

Thanks for making this explicit. I see requests for a "core build" from the Core team in Slack. I think we need to keep being explicit and own what we need rather than assume other teams will remember which components we need. Everyone manages multiple components and it's trivial to forget another team needs specific components. When requesting artifacts prior to a release, ask for all the artifacts needed, which I think for Platform is more than a core build and typically: stellar-core, stellar-xdr, soroban-env, soroban-sdk.

I think it would probably be fine to expand SDK's committer set

I don't think changing who is responsible for updating the SDK will help this situation. Doing so may make coordination and the interface between teams harder to navigate.

When planning for a pre-release cut of products, we might benefit from treating it much like a release event, possibly even target a tag that doesn't get published to crates.io. If we have a page similar to the releases docs page for these internal releases, it might help catch missing components earlier. There's enough to coordinate that I think it warrants the process. Wdyt @graydon? @anupsdf?

based on the temp commit

This mistake is easy to make, and we've made the same mistake before. We may be able to add a CI check to the rs-soroban-env and rs-soroban-sdk repos to prevent it in the future: https://github.com/stellar/actions/issues/55. It's a rare mistake, but the impact is significantly confusing to others upstream so I think it's worth it.

2opremio commented 1 year ago

when the core team is obliged to produce such a synchronized 5-tuple

In general, every time soroban-tools is updated with respect to Core (yeah, I know this isn't very helpful)

In practice, this happens every time there is a (substantial) XDR update.

As a rule of thumb, whenever we (platform) are awaiting for a new Core build (@sisuresh has been providing these) we also need the rs-env commit and a matching SDK update.

. A policy around this might make expectations clearer and reduce frustration

Yes, I agree. An official policy on how things should be updated would help. I was hoping this wouldn't be necessary but after having reached this point, it probably is.

2opremio commented 1 year ago

would you like to have rights to push updates to SDK if we've failed to update it, i.e. when it's a revision or two behind the more closely-coupled xdr + rs-xdr + env + core set?

Having write permissions to the Rust repos would help (e.g. it would had allowed me to restore the branch pointing to the dangling commit here and move forward)

However, in the particular case of the SDK, it can be more complicated than simply bumping it's xdr/rs-env dependencies to the desired ones.

In this case, the SDK had already "overtaken" the rs-env dependency we were targetting, requiring a downgrade. We didn't want to downgrade the main repo so @sisuresh created a separate branch in his fork https://github.com/sisuresh/rs-soroban-sdk/tree/ee389 . As a result, soroban-tools (even if temporarily) now depends on @sisuresh 's fork, which is less than ideal

I don't want to find myself downgrading the SDK.

sisuresh commented 1 year ago

The issue in this specific case was that the core team was making a lot of changes while we were trying to get a build out, and the result was that the sdk was bumped to a version of env that was past what the build used. Going forward, our policy should just be that every env change should be followed by an sdk change (and if that's too burdensome, make sure every core build we give to platform has a matching sdk commit). We can mitigate mistakes here by always giving platform the core version, env hash, and the sdk hash.