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 official MU4 ARMhf and ARM64 Linux github actions #15923

Merged
merged 9 commits into from
Jan 23, 2023

Conversation

theofficialgman
Copy link
Contributor

@theofficialgman theofficialgman commented Jan 16, 2023

Resolves: #15251

armhf and arm64 AppImages built using github actions. Full integration with the current github building and publishing stack has been accomplished and included in this PR.

ARMhf and ARM64 appimages are useful in a world where ARM adoption is growing at a rapid pace. Asahi linux running on Apple Silicon Macs, 46 million Raspberry Pi computers sold, a whole host of ARM SBCs, ARM computers in the embedded space and server world, game consoles (Nintendo Switch has sold 115+Milion units), and don't forget the 30+ million chromebook sales per year (yes all of the new ones armhf/arm64/x86_64 can run appimages through the linux compatibility layer)

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • N/A - I created a unit test or vtest to verify the changes I made (if applicable)

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Jan 16, 2023

CC: @Botspot @AntonioBL

scheduled builds are disabled as they take a long time (no ccache) and really are not necessary since x86_64 already has scheduled builds. updating translations in the build is also disabled as it is already covered by the x86_64 build and can't be run on armv7l/aarch64 (no builds for these architectures)

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Jan 16, 2023

there is one issue @Eism , disabling the update module (-DBUILD_UPDATE_MODULE=OFF) causes build failures. see here: https://github.com/theofficialgman/MuseScore/actions/runs/3933939669/jobs/6728175115#step:6:8210

and enabling it will incorrectly download the x86_64 package when a new version is released (or whatever it sees first on the storage repo) since https://github.com/musescore/MuseScore/blob/master/src/update/internal/updateservice.cpp does not do architecture checking. it only matches the filetype

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Jan 17, 2023

See #15623, which should include a fix for disabling the update module.

(Oh wait, you're already doing this on the master branch, not 4.0.1... then it's apparently broken again. Will take a look.)

@cbjeukendrup
Copy link
Contributor

scheduled builds are disabled as they take a long time (no ccache)

I don't think ccache is the reason for this; I believe that due to the way GitHub Actions Cache works, ccache doesn't work at all in the existing workflows either. So I think the reason is that everything has to run emulated, and that some tools need to be built from source.

@igorkorsukov
Copy link
Contributor

@theofficialgman Why did you make all sh files executable?

@AntonioBL
Copy link
Contributor

AntonioBL commented Jan 17, 2023

I think not all the sh need to be executable. For the scripts in this PR to run it is maybe needed only for
build/ci/linux_ARM/setup.sh
build/ci/translation/run_lrelease.sh
build/ci/linux/build.sh
build/ci/linux/package.sh
build/ci/tools/checksum.sh
that are launched as a string of commands for /bin/bash.

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Jan 17, 2023

See #15623, which should include a fix for disabling the update module.

(Oh wait, you're already doing this on the master branch, not 4.0.1... then it's apparently broken again. Will take a look.)

@cbjeukendrup actually I linked you an actions that I had based on 4.0.1. I never let actions for this master based PR complete with the update module disabled. I will do that now for testing and check back in a few hours. https://github.com/theofficialgman/MuseScore/actions/runs/3939838375

@theofficialgman
Copy link
Contributor Author

@theofficialgman Why did you make all sh files executable?

to avoid needing to do /bin/bash path/to/script and just allow executing via path/to/script. some scripts are already executed this way and its typical linux fasion that the scripts be marked executable so that it can be done as such. I figured now was as good a time as ever to make this correction across the whole repo.

@igorkorsukov
Copy link
Contributor

@theofficialgman Why did you make all sh files executable?

to avoid needing to do /bin/bash path/to/script and just allow executing via path/to/script. some scripts are already executed this way and its typical linux fasion that the scripts be marked executable so that it can be done as such. I figured now was as good a time as ever to make this correction across the whole repo.

I intentionally left them unexecutable so that there was less chance of making a mistake, accidentally run it, and so on
Therefore, I was a little surprised why all the scripts had their rights changed :)

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 17, 2023

If they are only ever executed from within another script, leaving them unexecutable seems a sane choice, for the reasons gven.

A way to ensure that is to have them start with #!/bin/false ;-)

@theofficialgman
Copy link
Contributor Author

I intentionally left them unexecutable so that there was less chance of making a mistake, accidentally run it, and so on Therefore, I was a little surprised why all the scripts had their rights changed :)

yeah thats the thing. they were not all unexecutable. it was a mishmash of some being executable and some not being executable. I don't really see how one could make the mistake of accidentally running a script.

it is minimal efffort, I can drop that commit and add bash to the execution in the arm yml.

I will get to the the other comments later today. Thanks for the feedback everyone. 😃

@theofficialgman
Copy link
Contributor Author

@cbjeukendrup @Eism disabling the update module does work on the master branch (see my actions test below). Sorry for the false alarm
https://github.com/theofficialgman/MuseScore/actions/runs/3939838375

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Jan 18, 2023

the latest changes in this PR should have resolved all the existing comments

testing again here: https://github.com/theofficialgman/MuseScore/actions/runs/3944414751

@theofficialgman
Copy link
Contributor Author

scheduled builds are disabled as they take a long time (no ccache)

I don't think ccache is the reason for this; I believe that due to the way GitHub Actions Cache works, ccache doesn't work at all in the existing workflows either. So I think the reason is that everything has to run emulated, and that some tools need to be built from source.

what I meant here was that ccache won't work inside the docker container for this arm action. so it was disabled. linux/macos/windows builds run on a schedule and complete fast enough (20 minutes or so) but these builds, because they are emulated, take 2-5 hours (depending on what runner picks them up). So we don't really want to run them on a schedulle constantly and it isn't really necessary to do so either since linux x86_64 is already running on a schedule.

building the tools themselves takes ~1hr on its own. upstream does not have armv7l or aarch64 appimages for linuxdeploy (or any of its plugins).

@shoogle
Copy link
Contributor

shoogle commented Jan 18, 2023

@theofficialgman

I have not [signed the CLA] but intend to do so if I can do so using my github username. I do not have any ties between github and my real name or address which your CLA seems to want.

To sign the CLA, you must create/login to an account on musescore.org, then visit musescore.org/cla and answer the questions truthfully, with real information, except you may leave certain answers blank if you have nothing to write there (e.g. fax number, corporate contributor information).

Your answers are not made public and they are not linked to GitHub in any way. However, you would need to tell us your GitHub username if it is different to your musescore.org username. One way to do this is:

  1. Visit your musescore.org profile page (https://musescore.org/user).
  2. Click the 3 dots button > 'Edit profile'.
  3. Specify your GitHub account name in the relevant field.

Please note that doing this will cause the GitHub account name to be publicly displayed on your profile page at https://musescore.org/user. If you want to avoid that, write "My GitHub username is theofficialgman" in the corporate contributor information field on the CLA form instead of specifying a GitHub account in your https://musescore.org/user profile settings.

@theofficialgman
Copy link
Contributor Author

@shoogle done, thanks for the info

@theofficialgman theofficialgman changed the title add official MU4 ARMhf and ARM64 github actions add official MU4 ARMhf and ARM64 Linux github actions Jan 18, 2023
@igorkorsukov
Copy link
Contributor

@theofficialgman Great job!

I ask you to update with the master, the failed test was temporarily disabled there until the reasons are clarified.

theofficialgman and others added 4 commits January 19, 2023 08:33
Co-Authored-By: Antonio Lotti (ABL) <2855332+AntonioBL@users.noreply.github.com>
only use appimageupdatetool if update information is set.
use QT_PATH directly for copying QtQuick

Co-Authored-By: Antonio Lotti (ABL) <2855332+AntonioBL@users.noreply.github.com>
@theofficialgman
Copy link
Contributor Author

@igorkorsukov links replaced. re-ordered and squashed some commits for clarity. no other code changes

@igorkorsukov igorkorsukov merged commit 4d4b42f into musescore:master Jan 23, 2023
@cbjeukendrup cbjeukendrup added this to Needs Triage in [MU4.0.2 PATCH-RELEASE] via automation Jan 25, 2023
@theofficialgman
Copy link
Contributor Author

if anyone is looking to do a clean rebase/pick to 4.0.2
you will need the stubs update PR: #15623
as well as the commits named
Don't install crashpad handler if `BUILD_CRASHPAD_CLIENT` is disabled
Add option to specify custom crash_handler path
Fix linking on Linux without Crashpad Client

@RomanPudashkin RomanPudashkin removed this from Needs Triage in [MU4.0.2 PATCH-RELEASE] Jan 27, 2023
theofficialgman added a commit to Botspot/pi-apps that referenced this pull request Jan 30, 2023
includes crashpad and appimageupdate (CLI updater) binaries in the appimage. builtin GUI updater disabled now on armv7l and aarch64 (non functional for these architectures).
now based on the merged upstream PR: musescore/MuseScore#15923
@MarcSabatella
Copy link
Contributor

With 4.0.2 now imminent, I gather this won't be merged for the main release. But my understanding is it should still be possible to add it to a new 4.0.2 branch afterwards and then manually generate what is then essentially a 4.0.2 build for ARM. If that happens, great, otherwise, I'd love to see another "unofficial" build. My assumption is that unless there is some really critical regression found, 4.0.2 may well be the last patch release for a while, so it would be good to have an ARM build.

@theofficialgman
Copy link
Contributor Author

@MarcSabatella I plan on making one once 4.0.2 releases
if this PR doesn't get implemented I will also be including it since it is such a huge improvement #16399 (fixes my reported bug #15982)

@AdamSEY
Copy link

AdamSEY commented Mar 14, 2023

@theofficialgman please let us know when it's ready.

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Mar 14, 2023

@theofficialgman please let us know when it's ready.

@AdamSEY it is building rn.
https://github.com/theofficialgman/MuseScore/actions/runs/4421630884

In addition to the required changes, I also picked the playback optimization to the branch since Musescore developers didn't include it in 4.0.2, it is vital for all systems and espeically low powered ones. v4.0.2...theofficialgman:MuseScore:v4.0.2arm_bionic

@MarcSabatella
Copy link
Contributor

MarcSabatella commented Mar 14, 2023

FWIW, I am a bit nervous about including a largely untested change in something purporting to be a 4.0.2 build. For me that change makes no difference at all in the performance of MU4 - on systems where it performs well, it continues to perform well, but on the systems where it does not, it continues to not. So I see no benefit personally, and see much potential downside if it ends up breaking something.

There are any number of other PR's that I'd frankly consider far more important to see included, and I hope there is a 4.0.3 if 4.1 takes more than just a few months. And I'd be fine with there being a completely unofficial 4.0.3 with those changes. But I'd prefer to see anything labeled 4.0.2 to be built from the same sources on all platforms.

@AdamSEY
Copy link

AdamSEY commented Mar 14, 2023

Glad to hear that! I would utilise it on an Ubuntu system running on an ARM processor, as the performance is significantly better than x86. I have tested version 3.6 on ARM and there is a noticeable improvement in performance.

@AdamSEY
Copy link

AdamSEY commented Mar 14, 2023

@theofficialgman I'm wondering if you had ever built a 4.0.* version before and if I can try it?

@theofficialgman
Copy link
Contributor Author

@theofficialgman I'm wondering if you had ever built a 4.0.* version before and if I can try it?

please look up in the thread a few posts. appimages for 4.0.1 were made around that time of release and can be installed with pi-apps

I don't directly link builds since links change but you can find it with a little effort

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Mar 14, 2023

FWIW, I am a bit nervous about including a largely untested change in something purporting to be a 4.0.2 build. For me that change makes no difference at all in the performance of MU4 - on systems where it performs well, it continues to perform well, but on the systems where it does not, it continues to not. So I see no benefit personally, and see much potential downside if it ends up breaking something.

MU4 performs like garbage without it... honestly I can't belive that MU4 released without the change included. I already made my decision and it is staying in. You are welcome to fork/build yourself ofc with the change removed. I much prefer to not have my CPU clocks pinned to 100% and my battery life and system performance go to crap but you do you.
incase you misssed the issue: #15982

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Mar 14, 2023

on systems where it performs well, it continues to perform well, but on the systems where it does not, it continues to not.

I really doubt that. Are you telling me that you tested on your system before and after the change? I did. That is what these builds are https://github.com/theofficialgman/MuseScore/actions/runs/4369667743 Tested on both my low powered arm systems as well as my powerful desktop. On both the issue was fully resolved. The build from before the commit had my intel cpu boosting to 4.8GHz at 100% load on one core and the cursor is not smooth, while after the cpu stays at base clock 0.8 Ghz with low utilization and the cursor movement is smooth

@AdamSEY
Copy link

AdamSEY commented Mar 14, 2023

@theofficialgman I'm wondering if you had ever built a 4.0.* version before and if I can try it?

please look up in the thread a few posts. appimages for 4.0.1 were made around that time of release and can be installed with pi-apps

I don't directly link builds since links change but you can find it with a little effort

I found 4.0.1 builds but none for arm64. I see here you built it on arm64, do you think you can share the AppImage build file?

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Mar 14, 2023

I found 4.0.1 builds but none for arm64. I see here you built it on arm64, do you think you can share the AppImage build file?

what you linked is master + cursor optimization picked before it got merged. and those two artifacts that you see are zips of that contain the appimages.

just wait for this complete and the 4.0.2 based zips that contain the appimages will be there https://github.com/theofficialgman/MuseScore/actions/runs/4421630884

@AdamSEY
Copy link

AdamSEY commented Mar 14, 2023

@theofficialgman I cannot wait for it to finish. I tried 4.0.1 on ARM64 and it works perfectly.

@MarcSabatella
Copy link
Contributor

Are you telling me that you tested on your system before and after the change? I did.

Yes, I am telling I tested precisely that (on my Intel systems), and saw/heard no change. On the system with the lower-powered processor, sound was choppy with our without the change, CPU usage was also similar.

Which is what makes me say, there is more going on here than meets the eye, and what makes me nervous about seeing this included with essentially no testing and no real understanding of what other factors could be going on to make the behavior differ between systems. The risk of untested changes is great, the reward to me is unknown as we don't know how many systems might actually see the sort of improvement you describe.

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Mar 14, 2023

The risk of untested changes is great, the reward to me is unknown as we don't know how many systems might actually see the sort of improvement you describe.

Don't use it then. That is your choice. I have made mine and I for one understand what the code changes are doing and accept them. The Musescore devs also understood and accepted them. PRs don't get approved and merged without reason.

I personally never had sound issues regardless of this. That isn't what this change is targeting anyway. The change makes it so that the entire score does not need to be redrawn every single frame which is a VERY compute intensive task. Instead only a small rectangle around the cursor is redrawn.

Did you look at HTOP on systems and check the CPU freq and the QSGRenderThread useage before and after the change?

Also if you have an issue with the PR, please take it there or the original issue, not here.

@MarcSabatella
Copy link
Contributor

MarcSabatella commented Mar 15, 2023

It's not about what I personally want to use - it's what I feel comfortable recommending to the thousands of users who come to my site looking for educational resources. I'm really reluctant to be recommending experimental software and misrepresenting it as something that has received the same level of testing as an actual release. Please, why not make two versions, a real 4.0.2 version, and an experimental cutting edge version with whatever additional PR's you like?

My issue isn't with the PR. I'm simply asking respectfully that you to consider performing this simple service to the MuseScore community. I'm commenting here because this is where the discussion of making 4.0.2 builds for ARM has been taking place, and you're the one who can make this happen.

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Mar 15, 2023

It's not about what I personally want to use - it's what I feel comfortable recommending to the thousands of users who come to my site looking for educational resources. I'm really reluctant to be recommending experimental software and misrepresenting it as something that has received the same level of testing as an actual release. Please, why not make two versions, a real 4.0.2 version, and an experimental cutting edge version with whatever additional PR's you like?
My issue isn't with the PR. I'm simply asking respectfully that you to consider performing this simple service to the MuseScore community.

I consider it a great disservice to provide something markedly worse on all accounts than what is available. This of this like how linux distros build their software on stable upstream releases but patch it with security fixes, optimizations, and bugfixes often derived from that piece of softwares upstream repositories. However, I am not willing to continue arguing the topic anymore. Know that the build that will be produced here (https://github.com/theofficialgman/MuseScore/actions/runs/4426527107) only has the necessary changes applied ontop of 4.0.2 tag. I will not be hosting this build elsewhere and I frequently disable actions on repositories on repos that I am not using. So I suggest downloading it when it finishes (~3hrs from now) for safekeeping and hosting it on your own platforms.

Know that if you recommend that build users will be getting an inferior product like the official 4.0.0, 4.0.1, and 4.0.2 that will boost CPU clock, drain battery life during playback, and have stuttery playback cursor on all but the most powerful systems.

I'm commenting here because this is where the discussion of making 4.0.2 builds for ARM has been taking place, and you're the one who can make this happen.

Documentation on how to use github desktop to cherry pick commits -> https://docs.github.com/en/desktop/contributing-and-collaborating-using-github-desktop/managing-commits/cherry-picking-a-commit

@theofficialgman
Copy link
Contributor Author

@RomanPudashkin @igorkorsukov @Eism Please give this ARM linux workflow a run on musescore 4.1.0. everything is in place in that branch

@MarcSabatella
Copy link
Contributor

What else would still need to happen in order to produce ARM builds for Linux from current master, and more importantly, to get actual official ARM builds for upcoming releases?

@theofficialgman
Copy link
Contributor Author

What else would still need to happen in order to produce ARM builds for Linux from current master, and more importantly, to get actual official ARM builds for upcoming releases?

@MarcSabatella we are running into multiple issues currently. You can read about them here -> #18575 (comment)
Unfortunately MuseScore made changes that made it impossible to build with GCC-7 anymore (outlined in that linked comment). Anything else (clang/gcc-8) causes QEMU to fail on linking (see upstream bug https://gitlab.com/qemu-project/qemu/-/issues/1763 )

I will look into building on focal instead of bionic (which is what it does currently for best distro compatibility) to see if that happens to change anything but I doubt it since this seems like a QEMU bug.

Github is in the planning stages for ARM64 Linux Hosted runners actions/runner-images#8439 (comment) but that will likely take months/years still before it is released. When that releases QEMU will not be necessary anymore.

I tried many times to implement cross compilation support in the actions but this proved very difficult for me so I gave up.

@theofficialgman
Copy link
Contributor Author

also looks like I managed to miss this PR that hopefully resolves the issue with building on GCC-7 #19268

I will test building that as well in the coming days

@radeva
Copy link

radeva commented Jan 18, 2024

@theofficialgman In case you need some free minutes on M1 and M2 MacOS runners, you can try FlyCI's M1 and M2 runners. They are on average 2x faster and 2x cheaper than GitHub's AND we have a free tier for OSS projects (see below).

Install Instructions

  1. Install FlyCI app and
  2. Easily replace one line of code and start using FlyCI runners:
jobs:
 ci:
-    runs-on: macos-latest
+    runs-on: flyci-macos-large-latest-m1
   steps:
   - name: 👀 Checkout repo
     uses: actions/checkout@v4

500 mins/month Free for Public Repos

Since your repo is public, FlyCI offers 500 mins/month of free M1 runner usage with the flyci-macos-large-latest-m1 runner for public projects.

Don't hesitate to contact us in case the free tier doesn't suit your needs or you experience any issues with the runners. Our team is here to support you!

Best Regards,
Veselina Radeva
Product Manager at FlyCI

@theofficialgman
Copy link
Contributor Author

theofficialgman commented Jan 18, 2024

@radeva Hi. I am requesting that you please stop the spam advertising of your product all over github. Refer to the github acceptable use policies https://docs.github.com/en/site-policy/acceptable-use-policies/github-acceptable-use-policies#10-advertising-on-github . You may not advertise in other Users' Accounts, such as by posting monetized or excessive bulk content in issues. I do not want to have to report you and your colleagues accounts (eg: @kgantchev) for violation.

Additionally, this is CI for LINUX ARMhf/ARM64 which your product does not offer.

@radeva
Copy link

radeva commented Jan 18, 2024

@theofficialgman I apologise for the approach. We considered we might be of help here. I wasn't aware of the mentioned policies, but will have it in mind.

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.

Feature request: ARM builds
9 participants