Enable Unity Build for Engraving module #8765

cbjeukendrup · 2021-07-29T21:19:56Z

It appeared that a #ifndef MU_..._H guard was missing, and for the rest, only the read114.cpp and read206.cpp files were conflicting with each other, so I enabled the Unity Build for the whole module and excluded only those critical files.
So, hopefully we get a little faster builds now :)

cbjeukendrup · 2021-07-29T21:46:36Z

Ah, the Windows build is failing. Again because of the same problem that Ashwath is having in his GSoC PR: in some Windows header, there is a #define small char. So when we have a function small(), it goes wrong.

It might be better anyway to rename small() functions to isSmall() or something even more descriptive, but I will first look for a real solution to the problem.

Jojo-Schmitz · 2021-07-30T06:40:55Z

IMHO the real solution is indeed to change small() to isSmall(). Adding yet another #undef small is much more of a (lean and mean) hack than anything else. It is not sufficient though, unfortunatley, as there are also some variables called small, also in .ui files, those all would need to get renamed too. I tried it with the 3.x branch: it is quite a big change!

Here we could get away with an #undef small in accidental.h (tested with latest master and your changes from the PR on top). The cheap way out...

It is not a plain Windows issue though, but rather an MSVC one, as MinGW does not have that issue.

Best if we could get rid of that #undef STRING_NONE too (in mscore.h). At least we should (again, like it was in 3.x's all.h) bracket it with #if (defined (_MSCVER) || defined (_MSC_VER)), to document that.

And that header guard is need in any case, it is the RightThing(tm), not just for Unity builds ;.)

I do get a warning with your PR (and the 'fix' for the compile error, see above):

Two or more files with the name of chordlist.cpp will produce outputs to the same location. This can lead to an incorrect build result.  The files involved are ...\src\engraving\libmscore\chordlist.cpp, ...\src\engraving\compat\chordlist.cpp.

Excluding those 2 from Unity build results in compiler errors:

20>.../src/engraving/libmscore/textlinebase.cpp(469,38): error C2371: 'Ms::pids': redefinition; different basic types (compiling source file ...\msvc.build_x64\src\engraving\CMakeFiles\engraving.dir\Unity\unity_12_cxx.cxx)
20>.../src/engraving/libmscore/textbase.cpp(2396): message : see declaration of 'Ms::pids' (compiling source file ...\msvc.build_x64\src\engraving\CMakeFiles\engraving.dir\Unity\unity_12_cxx.cxx)

So renaming at least one of those is due. With that it finally builds.

Complete (re)build in 1 minute on my machine ;-)
Without that change it takes 10 seconds longer.

Jojo-Schmitz · 2021-07-30T15:48:24Z

That's an option too (and cleaner!), but won't solve the warning I mentioned above (which does sound quite dangerous), and, once fixed, the naming conflicts I also mentioned

cbjeukendrup · 2021-07-30T15:52:18Z

I didn't like just putting #undef small in one place so that it just works (until it breaks again); I wanted to know what was going on exactly. So I put #undef small below every #include <windows.h>, in the whole project. And even that didn't solve the problem. It seems that it also comes via some Qt headers.

So I decided not to pollute the code with #undef small, and renamed the small functions and vars. That's indeed quite a lot of changes, but not too many in my opinion (and it fits better with our general naming practice anyway).

I did not encounter a warning or error about chordlist.cpp however, and can't seem to find anything about it in the CI build logs either.

On my two-core (!) Windows laptop, a full build (including unit tests) takes still almost 10 minutes (I don't know how long it takes without unity). But it's great to hear that it gives good results for you; that's in fact a 14% reduction.

Jojo-Schmitz · 2021-07-30T15:53:58Z

OK, let's see what it does on my machine (12 cores + HT) once merged. If I then still see that warning about the 2 chordlist.cpp files, I know what to do ;-)

Jojo-Schmitz · 2021-07-30T16:01:14Z

Oh, and I don't do the unit tests (haven't figured out how to, so I let GitHub CI do it for me ;-)
Remains the issue with #undef STRING_NONE

cbjeukendrup · 2021-07-30T16:01:55Z

Remains the issue with #undef STRING_NONE

True, I'll fix that.

cbjeukendrup · 2021-07-30T16:06:25Z

By the way, while looking at the CI log, I saw that there was a mistake in build_ninja.sh, so I fixed that too.

as needed for MSVC, which uses them in some headers Partial backport of musescore#8765

Jojo-Schmitz · 2021-08-01T09:30:33Z

src/plugins/api/elements.h

@@ -168,7 +168,7 @@ class Element : public Ms::PluginAPI::ScoreElement
    API_PROPERTY_T(bool,   visible,       VISIBLE)
    /** Stacking order of this element */
    API_PROPERTY_T(int,    z,             Z)
-    API_PROPERTY(small,                   SMALL)
+    API_PROPERTY(isSmall,                 SMALL)


This seems not needed and might break plugins using small

It does cause an mtest failure (in 3.x):

52/62 Test #53: tst_scripting ....................***Failed 1.09 sec This plugin does not support createPlatformOpenGLContext! --- /home/runner/work/MuseScore/MuseScore/mtest/scripting/p1.log.ref 2021-08-03 11:40:32.524648773 +0000 +++ p1.log 2021-08-03 12:05:47.534379914 +0000 @@ -4,13 +4,13 @@ found:TimeSig (24) at 0 found:Chord (93) at 0 beamMode:0 - small:false + small: stemDirection:AUTO duration:1/4 found:Rest (25) at 480 found:Chord (93) at 960 beamMode:0 - small:false + small: stemDirection:AUTO duration:1/2 found:BarLine (12) at 1920 <diff -u /home/runner/work/MuseScore/MuseScore/mtest/scripting/p1.log.ref p1.log failed

Okay, I'll undo this change! (and other plugin API related ones, if any)

as it was needed for MSVC, which uses these in some headers, so they conflict with MuseScore's functions, variables, marcos. Partial backport of musescore#8765

igorkorsukov · 2021-08-03T09:01:15Z

It seems, windows.h was in one place
#8785

Jojo-Schmitz · 2021-08-03T09:44:28Z

It seems, windows.h was in one place
#8785

Maybe, but I don't think this relates to this PR here. It is in main.cpp too (and a few other places, like filesystem.cpp)

igorkorsukov · 2021-08-03T09:51:26Z

It seems, windows.h was in one place
#8785

Maybe, but I don't think this relates to this PR here. It is in main.cpp too (and a few other places, like filesystem.cpp)

unit build does not combine all project files, but only files of one target (module)

Jojo-Schmitz · 2021-08-03T09:53:51Z

Still, as long as the conflicting small and STRING_NONE are not used (or #undefd), those (possible superfluos #include <windows.h>) shouldn't cause any trouble.

(It is defintly needed in fliesystem.cpp and certainly in main.cpp too)

igorkorsukov · 2021-08-03T10:07:42Z

Another solution, we can put fixed_windows.h somewhere and only include it :)
fixed_windows.h

#ifndef MU_FIXED_WINDOWS_H
#define MU_FIXED_WINDOWS_H

#include <windows.h>
#undef small
#undef STRING_NONE
#undef WARNING
#undef INFO
#undef ERROR
#undef QUESTION
#undef FILE_OPEN
...

#endif // MU_FIXED_WINDOWS_H

as it was needed for MSVC, which uses these in some headers, so they conflict with MuseScore's functions, variables, marcos. Partial backport of musescore#8765

another part of musescore#8765, not sure whether it breaks existing plugins?

as it was needed for MSVC, which uses these in some headers, so they conflict with MuseScore's functions, variables, marcos. Partial backport of musescore#8765

another part of musescore#8765, not sure whether it breaks existing plugins?

as it was needed for MSVC, which uses these in some headers, so they conflict with MuseScore's functions, variables, marcos. Partial backport of musescore#8765

another part of musescore#8765, not sure whether it breaks existing plugins?

as it was needed for MSVC, which uses these in some headers, so they conflict with MuseScore's functions, variables, marcos. Partial backport of musescore#8765

Jojo-Schmitz · 2021-08-03T13:16:16Z

Rebase needed too (meanwhile)

cbjeukendrup · 2021-08-03T17:37:13Z

Rebase needed too (meanwhile)

I'm working on it (but I'm having some technical trouble at the moment...)

Also added some constexpr and updated a license header.

Jojo-Schmitz · 2021-08-07T09:36:56Z

As mentioned earlier this PR does result in an MSCV compiler warning, even after merge:

MSB8027	Two or more files with the name of chordlist.cpp will produce outputs to the same location. This can lead to an incorrect build result.  The files involved are 
...\src\engraving\libmscore\chordlist.cpp, 
...\src\engraving\compat\chordlist.cpp.

And fixing that by exluding those 2 from Unity build gives compiler errors reg. redefinition of Ms::pids in textlinebase.cpp and textbase.cpp, exactly as predicted...

Added fix for this to #8798

Jojo-Schmitz · 2021-08-07T09:41:33Z

There's another compiler warning, from MinGW, unrelated to this PR, but related to another change from @cbjeukendrup 👍

...\src\framework\ui\internal\platform\windows\windowsplatformtheme.cpp:44: Warnung: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'fnRtlGetNtVersionNumbers' {aka 'void (*)(long unsigned int*, long unsigned int*, long unsigned int*)'} [-Wcast-function-type]
In file included from ...\build.qtc\src\framework\ui\CMakeFiles\ui.dir\Unity\unity_0_cxx.cxx:13:
.../src/framework/ui/internal/platform/windows/windowsplatformtheme.cpp: In constructor 'mu::ui::WindowsPlatformTheme::WindowsPlatformTheme()':
.../src/framework/ui/internal/platform/windows/windowsplatformtheme.cpp:44:126: warning: cast between incompatible function types from 'FARPROC' {aka 'long long int (*)()'} to 'fnRtlGetNtVersionNumbers' {aka 'void (*)(long unsigned int*, long unsigned int*, long unsigned int*)'} [-Wcast-function-type]
         = reinterpret_cast<fnRtlGetNtVersionNumbers>(GetProcAddress(GetModuleHandleW(L"ntdll.dll"), "RtlGetNtVersionNumbers"));
                                                                                                                              ^

No idea what to do about this?

cbjeukendrup · 2021-08-07T10:12:32Z

I wonder why I have never seen that chordlist warning. Which generator do you use, Ninja or Visual Studio? I use Ninja, so that might explain the difference.

I'll try to fix the MinGW warning.

Jojo-Schmitz · 2021-08-07T10:14:14Z

VS not Ninja

I wouldn't even know how to use Nija (in VS or QtCreator), nor what the benefit might be?

as it was needed for MSVC, which uses these in some headers, so they conflict with MuseScore's functions, variables, marcos. Partial backport of musescore#8765

Jojo-Schmitz mentioned this pull request Jul 30, 2021

[MU4] Enable Unity Build for Engraving module #8770

Closed

cbjeukendrup force-pushed the engraving_unity branch from f222084 to d59b3cf Compare July 30, 2021 17:10

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 1, 2021

Eliminate #undef small and #undef STRING_NONE

d32927f

as needed for MSVC, which uses them in some headers Partial backport of musescore#8765

Jojo-Schmitz reviewed Aug 1, 2021

View reviewed changes

Jojo-Schmitz mentioned this pull request Aug 1, 2021

[MU3 Issue] MuseScore 3.6.3 or 3.7.0, PRs to be merged and issues to be fixed. #7449

Closed

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 3, 2021

Needed? Harmful?

c4cc98c

another part of musescore#8765, not sure whether it breaks existing plugins?

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 3, 2021

Needed? Harmful?

ca4b283

another part of musescore#8765, not sure whether it breaks existing plugins?

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 3, 2021

Needed? Harmful?

cd94520

another part of musescore#8765, not sure whether it breaks existing plugins?

cbjeukendrup force-pushed the engraving_unity branch from d59b3cf to 7042d67 Compare August 3, 2021 22:29

cbjeukendrup added 4 commits August 5, 2021 23:59

Fix naming conflicts in Windows build regarding small

2a7f778

Fixed naming conflict regarding STRING_NONE

7873665

Also added some constexpr and updated a license header.

Remove unnecessary MStyle dependency from mscore.h

02aa7b6

Fix ninja_build.sh

b4a4e41

cbjeukendrup force-pushed the engraving_unity branch from bc76e79 to b4a4e41 Compare August 5, 2021 21:59

RomanPudashkin approved these changes Aug 6, 2021

View reviewed changes

RomanPudashkin merged commit b7d8f7b into musescore:master Aug 6, 2021

cbjeukendrup deleted the engraving_unity branch August 6, 2021 09:44

Jojo-Schmitz mentioned this pull request Aug 7, 2021

[MU4] Fix compiler warnings #8798

Merged

Jojo-Schmitz mentioned this pull request Sep 1, 2021

[MU3] Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 #9000

Closed

Jojo-Schmitz mentioned this pull request Mar 1, 2023

Fix GH#7449: Almost everything suitable for 3.6.3 or 3.7.0 Jojo-Schmitz/MuseScore#80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Unity Build for Engraving module #8765

Enable Unity Build for Engraving module #8765

cbjeukendrup commented Jul 29, 2021

cbjeukendrup commented Jul 29, 2021

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

cbjeukendrup commented Jul 30, 2021 •

edited

Loading

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

cbjeukendrup commented Jul 30, 2021

cbjeukendrup commented Jul 30, 2021

Jojo-Schmitz Aug 1, 2021

Jojo-Schmitz Aug 3, 2021

cbjeukendrup Aug 3, 2021

igorkorsukov commented Aug 3, 2021

Jojo-Schmitz commented Aug 3, 2021

igorkorsukov commented Aug 3, 2021

Jojo-Schmitz commented Aug 3, 2021 •

edited

Loading

igorkorsukov commented Aug 3, 2021

Jojo-Schmitz commented Aug 3, 2021

cbjeukendrup commented Aug 3, 2021

Jojo-Schmitz commented Aug 7, 2021 •

edited

Loading

Jojo-Schmitz commented Aug 7, 2021 •

edited

Loading

cbjeukendrup commented Aug 7, 2021

Jojo-Schmitz commented Aug 7, 2021 •

edited

Loading

Enable Unity Build for Engraving module #8765

Enable Unity Build for Engraving module #8765

Conversation

cbjeukendrup commented Jul 29, 2021

cbjeukendrup commented Jul 29, 2021

Jojo-Schmitz commented Jul 30, 2021 • edited Loading

Jojo-Schmitz commented Jul 30, 2021 • edited Loading

cbjeukendrup commented Jul 30, 2021 • edited Loading

Jojo-Schmitz commented Jul 30, 2021 • edited Loading

Jojo-Schmitz commented Jul 30, 2021 • edited Loading

cbjeukendrup commented Jul 30, 2021

cbjeukendrup commented Jul 30, 2021

Jojo-Schmitz Aug 1, 2021

Choose a reason for hiding this comment

Jojo-Schmitz Aug 3, 2021

Choose a reason for hiding this comment

cbjeukendrup Aug 3, 2021

Choose a reason for hiding this comment

igorkorsukov commented Aug 3, 2021

Jojo-Schmitz commented Aug 3, 2021

igorkorsukov commented Aug 3, 2021

Jojo-Schmitz commented Aug 3, 2021 • edited Loading

igorkorsukov commented Aug 3, 2021

Jojo-Schmitz commented Aug 3, 2021

cbjeukendrup commented Aug 3, 2021

Jojo-Schmitz commented Aug 7, 2021 • edited Loading

Jojo-Schmitz commented Aug 7, 2021 • edited Loading

cbjeukendrup commented Aug 7, 2021

Jojo-Schmitz commented Aug 7, 2021 • edited Loading

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

cbjeukendrup commented Jul 30, 2021 •

edited

Loading

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

Jojo-Schmitz commented Jul 30, 2021 •

edited

Loading

Jojo-Schmitz commented Aug 3, 2021 •

edited

Loading

Jojo-Schmitz commented Aug 7, 2021 •

edited

Loading

Jojo-Schmitz commented Aug 7, 2021 •

edited

Loading

Jojo-Schmitz commented Aug 7, 2021 •

edited

Loading