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

Record of issues with merge contention in current dependency setup #4090

Open
ehildenb opened this issue Mar 11, 2024 · 4 comments
Open

Record of issues with merge contention in current dependency setup #4090

ehildenb opened this issue Mar 11, 2024 · 4 comments
Labels

Comments

@ehildenb
Copy link
Member

Please record issues with merge contention in current dependency setup here, and link to the specific PRs that were affected.

The goal is to understand which issues are solved by changing our CI/dependency setup, vs having better definition of teh API between tools.

@dwightguth
Copy link
Collaborator

dwightguth commented Mar 11, 2024

Here's a situation that occurred today that is pretty common (i.e., I encounter it on a weekly basis):

  1. This PR is merged: Enforce style llvm-backend#1000
  2. This PR fails CI: Update dependency: llvm-backend/src/main/native/llvm-backend #4076
  3. This PR is merged: add hook_LIST_push llvm-backend#1005
  4. This PR cannot be merged: add pushList function to LIST module in k prelude #4074 because it depends on an updated version of the LLVM backend that cannot be merged until K is updated to work with the PR in step 1.

This is a fairly fundamental issue of the order of commits where an incomplete, complex change is partially merged and thus a simple change that should be fast to merge is blocked on a much more complex change that takes weeks to resolve. I see this all the time and am happy to add every single instance of it that I encounter to this thread. So far as I'm aware, nothing proposed by @ehildenb in this issue would fix this type of stalled change.

@Baltoli
Copy link
Contributor

Baltoli commented Mar 15, 2024

Another scenario that's problematic is the backwards edges in the K dependency graph. We eliminated the one from the LLVM backend to K, but there's still one from the Haskell backend to K for running integration tests. When the K language / compiler behaviour changes, we need to close the loop somehow (either by making changes inline with the dependency update PR, or implementing a set of backwards-compatible PRs in sequence).

An example I've been working on:

  1. Deprecate symbol and klabel #4045 makes a change to the K -> KORE pipeline
  2. Usages of klabel{}(_) attribute remain haskell-backend#3742 arises; the Haskell backend breaks when this change is made
  3. Remove uses of klabel{}(_) attribute for special-casing symbols haskell-backend#3741 the HB fix for that issue can't be merged because it relies on Deprecate symbol and klabel #4045 being merged and used in the HB integration tests
  4. There's now a cycle between Deprecate symbol and klabel #4045 and Remove uses of klabel{}(_) attribute for special-casing symbols haskell-backend#3741

@dwightguth
Copy link
Collaborator

Here's another issue that's not related to merge conflicts but is significantly annoying and would be solved by putting the Haskell Backend and the Booster in the same repo: K builds the haskell backend twice on different stack resolvers any time a stack resolver for the booster is updated, until such a time as the stack resolver for the original haskell backend is also updated. This seems to happen pretty frequently and generally isn't something on the radar of the Haskell team to take care of, so it ends up lingering for some time each time.

@Baltoli Baltoli added the devops label Mar 26, 2024
@Baltoli
Copy link
Contributor

Baltoli commented Apr 16, 2024

Discussion from @dwightguth and @Scott-Guest on Slack:

@dwightguth: Here's another scenario that I am running into that is annoying. The llvm backend was updated with a change that was not tested against the K frontend. As a result, the latest version of the K frontend is broken with the latest version of the llvm backend. As a result, I am blocked on whatever is going on there when making my entirely unrelated change to the llvm backend. This happens because we are not making changes atomically. We either need to make repositories that contain every file needed for atomicity, or we need to not merge part one of a PR until part 2 has been tested. This is very disruptive to other people's development on the projects.

@Scott-Guest: Agreed, and I also feel pretty strongly that "test it yourself locally" is an insufficient solution.

  • The current breakage happened as follows:
    I made all necessary changes in my local monorepo and had a clean build which passed the regression test suite.
  • When I went to submit a PR for the scala-kore and llvm-backend parts, I ended up making a few additional modifications and merging in new things from master, but neglected to test against the frontend again after all this.
  • When the llvm-backend dependency update went through, there were then unexpected test failures in the fronted (caused by flakiness with parts of the codebase relying on set iteration order).
  • One of these test failures ended up being extremely annoying, and I still haven't managed to fix it, prolonging the breakage.
    That is to say, I did attempt to test against the frontend, but a small oversight in doing so has now lead to a lot of headaches.

https://runtimeverification.slack.com/archives/C7E701MFG/p1713215246074799

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants