[ci] now the submodule check checks for strict equality

[igorkorsukov]

Problem: The submodule may not be updated or point to a different fork.

Typical situation:

During development, the developer replaced the submodule with his own fork and forgot to change it back.
Some PR depends on changes in the MF. They were made and merged into the MF, but the submodule was forgotten or didn't know it needed to be updated. Although the test should have detected this.
Some fixes have been made in the MF, but no one has updated the submodule in the MU.

The AU guys provided us with a check to verify situation 1.
But I've now changed it to address situations 2 and 3.

The idea is that any PR will require the submodule to point to the latest commit, so the developer creating the PR will be forced to update the submodule. This is similar to the requirement to now fetch the latest upstream changes before creating a PR.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 191665ec-abcb-467b-84e5-bb18bbf0dee8

📥 Commits

Reviewing files that changed from the base of the PR and between b616cde and cf3d8ef.

📒 Files selected for processing (2)

• .github/workflows/check_submodules.yml
• muse

🚧 Files skipped from review as they are similar to previous changes (1)

• .github/workflows/check_submodules.yml

---

📝 Walkthrough

Walkthrough

This pull request modifies the GitHub Actions workflow that validates the muse_framework submodule. The change introduces an EXPECTED_BRANCH environment variable set to main and updates the verification step to enforce that the pinned commit SHA exactly matches the upstream HEAD commit for that branch. The previous verification logic, which checked whether the pinned commit was an ancestor of main, is replaced with a direct upstream HEAD comparison. The PR also updates the muse submodule reference to a new commit.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is entirely empty; no description was provided by the author.	Add a pull request description following the provided template, including issue reference, change motivation, and completion of required checklist items.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: the submodule check now validates strict equality of commits instead of ancestor checking.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cbjeukendrup]

Is it right that this forces every PR to update the submodule, even if the PR is unrelated to the latest submodule changes, and it would take much effort to perform the update? I'm not sure if that's so ideal.

[igorkorsukov]

Is it right that this forces every PR to update the submodule, even if the PR is unrelated to the latest submodule changes, and it would take much effort to perform the update? I'm not sure if that's so ideal.

In short, yes, this requires an update from everyone.
It's not ideal; it would be good to rely on the developer making the changes to the MF, but they might miss this.

To update the submodule we only need to run

git submodule update --init --remote ./muse

Problem: The submodule may not be updated or point to a different fork.

Typical situation:

1. During development, the developer replaced the submodule with his own fork and forgot to change it back.
2. Some PR depends on changes in the MF. They were made and merged into the MF, but the submodule was forgotten or didn't know it needed to be updated. Although the test should have detected this.
3. Some fixes have been made in the MF, but no one has updated the submodule in the MU.

The AU guys provided us with a check to verify situation 1.
But I've now changed it to address situations 2 and 3.

The idea is that any PR will require the submodule to point to the latest commit, so the developer creating the PR will be forced to update the submodule. This is similar to the requirement to now fetch the latest upstream changes before creating a PR.

[cbjeukendrup]

A disadvantage would be the following situation:

1. Developer A makes a change in the framework, required by work on an application
2. Developer B makes an independent PR to an application, and is now forced to update the framework, and also has to make all changes to the application that are required after the framework changes
3. Developer A tries to make his PR to the application, but finds that the framework update and related changes have already happened in a different PR, resulting in a very weird partial PR because part of the changes has already been integrated via a different unrelated PR.

Therefore I would suggest not strictly enforcing this check, i.e. don't fail, but post a warning comment. This way, each PR author can consider whether the framework should be updated in their PR, or that that's someone else's responsibility.

It's also not great when a check can suddenly fail based on external circumstances. For example, when you open the PR, everything might be fine, but when you then push a second commit, the check suddenly fails because apparently there has been an unrelated framework change in the meantime.

[igorkorsukov]

Today, we encountered a situation where the application simply wouldn't launch because we merged the changes but didn't update the MF.
(A field was added to the Qml component in MF, and the PR was using it. After merging it, the application wouldn't launch because the field was missing.)

This seems like a more critical issue. What other solutions are there?

[cbjeukendrup]

I suppose something like that should have been detected by CI or QA before the MU/AU PR was merged

[igorkorsukov]

Updating a submodule is like requiring we to update your local repository from upstream before PR.

[igorkorsukov]

I understand your concerns.
And many things should be resolved automatically if the correct process is followed.
But processes fail for various reasons.

I'd prefer a strict check right now, but we'll closely monitor what's happening, and if it does create problems, we'll change it.

[cbjeukendrup]

Updating a submodule is like requiring we to update your local repository from upstream before PR.

I'm not sure I agree. Updating a submodule (or any dependency) is a deliberate change, that you make at the moment that it makes sense and that you are ready for it. You wouldn't update FluidSynth either in every PR that touches something completely unrelated.
Updating your local repo with upstream commits is different, because that does not force you to include unrelated changes in your PR.

[cbjeukendrup]

Anyway, this makes the process of contributing yet more complicated, and will result in a big mess because submodule updates will be detached from the MU/AU PRs that they were related to.

[igorkorsukov]

I'd prefer to think of this now not as updating a submodule because it includes dependent changes.
But as simply keeping the code always up-to-date.
Like updating a local repository.

We are just learning to adapt to the new process, it is quite possible that you will be right... we will try.

[cbjeukendrup]

Updating the submodule should be an intentional step, because if you do it blindly like keeping your local repo up to date, you get broken builds. In fact, right now, the master branch does not launch, because the submodule was updated but the corresponding MuseScore PR was not merged. See musescore/muse_framework#6; the framework should not have been updated with that change until (the first commit of) #33301 was merged.

[cbjeukendrup]

I've created #33333 as a hotfix, so we don't have to wait for #33301 and its dependencies to be merged

[krasko78]

@igorkorsukov When can we expect the promised instructions on the dev and PR process now that the muse framework is a separate repo?

[igorkorsukov]

@krasko78 We're currently learning and gaining new experience working in a new way.
We're getting a picture, but not everything is clear.
We'll probably encounter a number of problems and deal with them.

And then we'll write instructions on how to work under the new conditions.
I believe we have a responsive community, and we can tolerate misunderstandings and inconveniences right now :)

[igorkorsukov]

Updating the submodule should be an intentional step, because if you do it blindly like keeping your local repo up to date, you get broken builds. In fact, right now, the master branch does not launch, because the submodule was updated but the corresponding MuseScore PR was not merged. See musescore/muse_framework#6; the framework should not have been updated with that change until (the first commit of) #33301 was merged.

I think this was a pretty simple case; we just needed to make the PR in MF as a draft. Then we wouldn't have merged it before the changes were merged into the MU repository.

[igorkorsukov]

Updating the submodule should be an intentional step

I don't think we'll be able to keep up with this. There are too many people.
It's the same as breaking master and saying, "Don't upgrade from master until we tell you it's okay."

[krasko78]

@krasko78 We're currently learning and gaining new experience working in a new way. We're getting a picture, but not everything is clear. We'll probably encounter a number of problems and deal with them.

And then we'll write instructions on how to work under the new conditions. I believe we have a responsive community, and we can tolerate misunderstandings and inconveniences right now :)

Gotcha! :) But even so, you can share what you have so far. :) For instance: a dev is working on a story that requires changes in MF. How can the dev test them locally? I saw a setting for the local path to the MF repo - when and how is it supposed to be used? Then, when the dev is happy with the changes, he/she commits them to the MF repo and how is the submodule then updated in MuseScore? Same PR or separate PR? All the PRs I see everyday like the one we are writing in right now, only add to the confusion. :) Or am I being too impatient? :)

[igorkorsukov]

@krasko78 see #33335 :)

[cbjeukendrup]

@igorkorsukov Some additional thoughts occurred to me:

• if we don't force the framework to be updated in every PR, things might break, because a PR author forgets to update the submodule (in most cases, this should be caught by CI and QA, but sometimes that might fail).
• if we do force the framework to be updated in every PR, things might also break, because someone blindly updates the submodule but forgets to update the app code accordingly. (That's what happened today with my PR.)

So, either way, things may break, equally likely. The difference is that if we do force updates in every PR, an update might appear in a PR where it is completely unrelated, resulting in messy history.
So the advantage of forcing updates turns out to be small or none at all, while the disadvantage still stands.

However, I can understand the desire to always keep the framework always up to date as soon as possible. How about a different strategy: create a GH Action that always creates an "Update framework" PR as soon as any commit is pushed to the framework repo. Then someone reviews this PR; if no app code changes are required, the PR can just be merged; if code changes are required, any maintainer can push those changes to the auto-generated PR, or create a PR manually to replace the autogenerated PR.

To me, that sounds like a good balance between always staying on the latest framework and keeping framework updates intentional / conscious, without disrupting other peoples PRs with forced unrelated framework updates. What do you think?

[igorkorsukov]

It's an interesting idea, I think it's definitely worth keeping in mind.

I had some thoughts about it too (or rather, I remembered them).

## How do we feel about MF

There are at least two approaches to MF:

1. MF is a separate product (like Qt).
In this case, it develops independently, has versioning and release cycles, is stable (it doesn't break in minor versions), has its own testing system, including a sample application, and so on. 

2. MF is simply a shared component of two products, MuseScore and Audacity. Any change is made to improve one or both products. Any change takes into account the impact on these two products.


We're currently choosing the second approach.
But in the future, it's entirely possible we'll move toward the first approach.

We're actually getting closer to the second approach now.
But I believe that over time we'll move toward the first approach. And then we won't need to somehow link the changes in the MF and the Applications. They should be considered separately.

[igorkorsukov]

I'm also concerned that the MF using two projects. They need to be synchronized and the same strategy used in both.