MuseScore icon indicating copy to clipboard operation
MuseScore copied to clipboard

add official MU4 ARMhf and ARM64 github actions

Open theofficialgman opened this issue 2 years ago • 12 comments

Resolves: #15251

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

theofficialgman avatar Jan 16 '23 23:01 theofficialgman

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 avatar Jan 16 '23 23:01 theofficialgman

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

theofficialgman avatar Jan 16 '23 23:01 theofficialgman

See https://github.com/musescore/MuseScore/pull/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 avatar Jan 17 '23 06:01 cbjeukendrup

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.

cbjeukendrup avatar Jan 17 '23 06:01 cbjeukendrup

@theofficialgman Why did you make all sh files executable?

igorkorsukov avatar Jan 17 '23 08:01 igorkorsukov

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.

AntonioBL avatar Jan 17 '23 09:01 AntonioBL

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 avatar Jan 17 '23 13:01 theofficialgman

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

theofficialgman avatar Jan 17 '23 13:01 theofficialgman

@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 :)

igorkorsukov avatar Jan 17 '23 14:01 igorkorsukov

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 ;-)

Jojo-Schmitz avatar Jan 17 '23 14:01 Jojo-Schmitz

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 avatar Jan 17 '23 16:01 theofficialgman

@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 avatar Jan 17 '23 17:01 theofficialgman

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 avatar Jan 18 '23 00:01 theofficialgman

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

theofficialgman avatar Jan 18 '23 00:01 theofficialgman

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

shoogle avatar Jan 18 '23 13:01 shoogle

@shoogle done, thanks for the info

theofficialgman avatar Jan 18 '23 14:01 theofficialgman

@theofficialgman Great job!

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

igorkorsukov avatar Jan 19 '23 12:01 igorkorsukov

@theofficialgman Great job!

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

@igorkorsukov rebased. I am waiting for an arm64 actions to complete at my fork since the last one failed due to missing a breakpad binary https://github.com/theofficialgman/MuseScore/actions/runs/3959032992 I built and added one here but you should probably re-host this file on the musescore server like the x86-64 file (https://s3.amazonaws.com/utils.musescore.org/breakpad/linux/x86-64/dump_syms.7z) https://github.com/musescore/MuseScore/pull/15923/commits/8002111f87f1ad3d2ad97537bcdf01f2bc84b837#diff-027df3ff509e7fd1b2b5966e1ad2e0df9877e753174b7437448f783ecb90a003R327-R336

theofficialgman avatar Jan 19 '23 13:01 theofficialgman

Thank you @theofficialgman !

AntonioBL avatar Jan 19 '23 14:01 AntonioBL

the run completed and symbols were generated for the aarch64 artifact (as it is the only one that has crashpad and dump_syms). I used google upstream crashpad with this one patch applied when building the binaries as it was much easier than building off of your fork https://github.com/musescore/crashpad_fork/commit/54528e00ef4ac46b0dcfe557bbb8f82fabfafe95

the artifacts are separated (by accident) but I like this better than having them in one so I made it deliberate with the above commit (now Lin_armv7l and Lin_aarch64)

https://github.com/theofficialgman/MuseScore/actions/runs/3959032992

I can't really test the publishing of the packages but I believe everything has been implemented correctly for that process to work so this is ready for merge (besides needing the aarch64 dump_syms zipfile to be hosted at musescore instead of my repo). Maybe in the future I will be able to get crashpad/dump_syms to build on armv7l.

I hope these releases can be included in a future version of MuseScore

theofficialgman avatar Jan 19 '23 19:01 theofficialgman

well you waited too long to merge this PR.... I got crashpad (and breakpad) to compile on armv7l natively (with a lot of coaxing with google's buildfiles and building gn from source)

latest run (with artifacts) is here for anyone that wants to test: https://github.com/theofficialgman/MuseScore/actions/runs/3964402069 youtube_api_key isn't available in my fork so I believe it is expect that no images show up for that. not sure if it is working in master at all

LGTM!

theofficialgman avatar Jan 19 '23 23:01 theofficialgman

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

@igorkorsukov rebased. I am waiting for an arm64 actions to complete at my fork since the last one failed due to missing a breakpad binary https://github.com/theofficialgman/MuseScore/actions/runs/3959032992 I built and added one here but you should probably re-host this file on the musescore server like the x86-64 file (https://s3.amazonaws.com/utils.musescore.org/breakpad/linux/x86-64/dump_syms.7z) 8002111#diff-027df3ff509e7fd1b2b5966e1ad2e0df9877e753174b7437448f783ecb90a003R327-R336

I copied "https://github.com/Pi-Apps-Coders/files/releases/download/large-files/dump_syms.zip" to "https://s3.amazonaws.com/utils.musescore.org/breakpad/linux/aarch64/dump_syms.zip"

and "https://github.com/Pi-Apps-Coders/files/releases/download/large-files/dump_syms_armv7l.zip" to "https://s3.amazonaws.com/utils.musescore.org/breakpad/linux/armv7l/dump_syms.zip"

Could you please replace the link

igorkorsukov avatar Jan 21 '23 10:01 igorkorsukov

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

theofficialgman avatar Jan 21 '23 15:01 theofficialgman

if anyone is looking to do a clean rebase/pick to 4.0.2 you will need the stubs update PR: https://github.com/musescore/MuseScore/pull/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

theofficialgman avatar Jan 26 '23 00:01 theofficialgman

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.

MarcSabatella avatar Mar 10 '23 23:03 MarcSabatella

@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 https://github.com/musescore/MuseScore/pull/16399 (fixes my reported bug https://github.com/musescore/MuseScore/issues/15982)

theofficialgman avatar Mar 11 '23 00:03 theofficialgman

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

AdamSEY avatar Mar 14 '23 19:03 AdamSEY

@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. https://github.com/musescore/MuseScore/compare/v4.0.2...theofficialgman:MuseScore:v4.0.2arm_bionic

theofficialgman avatar Mar 14 '23 20:03 theofficialgman

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.

MarcSabatella avatar Mar 14 '23 20:03 MarcSabatella

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 avatar Mar 14 '23 20:03 AdamSEY