Build system updates for Buildbot #1128
Conversation
|
I only took a very quick look, focussing on the macOS build. I will take a better look in a couple of weeks (I have little time to do it before I will be traveling). I suppose the And I might be wrong but to me it looks like something is missing to generate a static build of scummvm. I didn't see how the changes in configure fully replace the scummvm-static target. I am mostly noting this here as a remember to myself to check this when I get more time. I also see the osxsnap target has been removed. Is there something else in place to generate a macOS package easy to distribute? The parts where it sets the extended attributes on the UTF-8 files is useful for example as otherwise they are not displayed correctly when you open those on some versions of macOS. If we have to do all that manually this is prone to errors. By the way if you remove the osxsnap target, then I also see you removed the packaging for the old iPhone backend. Is there actually a value in keeping this old backend around or should the ios7 backend be a requirement now? If we want the old backend to stay around, shouldn't we also keep the packaging part of it? Or was there a good reason to remove it? |
|
Thanks for your feedback, these are great questions and definitely show some areas which could be improved.
That is an ultimate goal; this PR is a little short of it though. Right now a regular build links statically only because the Buildbot image has no shared libraries so the linker is forced to do it that way. More work would be needed to get this to work correctly everywhere since the Apple linker really doesn’t want developers to link anything statically. The static targets went away because libraries were hand-coded for the alternative target, so were fragile and got out-of-sync easily with the main LIBS list generated by
This is a good question. I honestly didn’t give a ton of thought to it. It seemed to me at the time that giving a zip file with the app bundle was sufficient, since this is how nearly all of the non-Mac-store apps I download these days are distributed. I haven’t seen apps distributed with extra files in the DMG like this quite some time, and the limitations of such an approach are fairly substantial (i.e. these extra files are only available so long as the installer image is still on the computer and mounted). I think we’d be much better off spending time adding a Help menu which links to the user manual instead, since that would give users access to this information at any time after installation, in the officially blessed OS-standard manner, and would eliminate the need for extra DMG stuff just for docs. This would be needed if we were ever to publish to the Mac App Store anyway. What do you think about doing this instead of trying to restore DMG packaging?
Noted. Once the previous question is resolved I will update this to remove them if that is the path we take.
IIRC, I thought that the packaging for that was just broken due to the way the macOS stuff was commingled with it, but I may have been just confused. In any case I don’t think there is any reason to retain that old backend, there haven’t been any downloads (literally zero non-bot downloads) for any iPhoneOS packages in a year and devices going all the way back to iPhone 4 should work with the newer iOS backend according to iOS support matrix. So I guess not keeping any of it around any more makes sense to me. |
That is not actually correct. The static target does not do a full recompilation. If all the objects are already compiled then it only does one link to generate the statically-linked executable.
The majority of the dmg images indeed come with just the app bundle and a shortcut to the Applications folder (which we don't even have). It is not uncommon though to also have a ReadMe file.
That is something that has been on my list for a while actually. But even assuming the user manual on the wiki has all the content currently in the ReadMe file, it would not replace the distribution of the NEWS file and translated QuickStart/ReadMe (unless they are all moved to the wiki as well). Although those could be included inside the app Bundle and accessed from the Help menu (I have just added that idea to my list). And then there is the licenses (GPL, LGPL, BSD and FREEFONT), copyright, and author files. Is that fine to also have those hidden the bundle with access from the menu? |
You’re right, I only remembered that it wasn’t using the
Until we can all sit down and figure out how to get to a single documentation source, it’s completely reasonable to me to have the Help menu items just open the same documentation files, which would now live in the bundle. As far as licensing goes, there is no prescribed way in which licences must be made visible, the only requirement is that they must be included with the program, so a link on the Help menu is fine. If this all sounds OK, I can work on making this change, unless you’d like to do it first. Let me know. (As an aside, I’d also really like the About window to not be an auto-scroller; it gives all this license and author information, but is unusable.) |
configure
Outdated
| append_var CXXFLAGS "-Wuninitialized" | ||
| elif test "$_optimizations" != no && cc_check_no_clean -Og; then | ||
| echo "-Og" |
There was a problem hiding this comment.
Are you sure about setting -Og by default? It's good for us for building faster debug builds, but won't linux packages maintainers hate it? For example the Gentoo people like setting very specific compiler flags via environment variables.
There was a problem hiding this comment.
I’m not very confident. I’ve waffled back and forth on the best non-release optimisation strategy, and on whether or not Buildbot should just always generate fully optimised builds since users today use these builds as rolling releases. It seemed like something needed to be done differently since -O0 isn’t good enough for Munt, -Os wastes server CPU time & ruins debugging by optimising out symbols, and so -Og seemed like it could be a decent compromise between runtime speed, compilation speed, and avoiding symbols optimising away. As a result of my doubt, I added the user override so it is possible to force the strategy with e.g. --enable-optimizations=-O0 if needed. I’m interested in all arguments one way or the other here.
There was a problem hiding this comment.
Sorry, my comment was not clear enough. -Og seems like a good compromise to me for debug builds.
What I'm not sure about is if configure should set any optimization flag by default. Linux packaging tools usually set the optimisation flags in the environment themselves. If we override that with -Og, people will expect to have release builds but instead get debug-optimized builds.
There was a problem hiding this comment.
I've looked at the ScummVM Gentoo ebuild and it doesn't seem to use the system's C[XX]FLAGS specified in /etc/make.conf right now. Usually, it is expected that ebuilds honor the environment variables to an extent that makes sense for the given package. When upstream doesn't use autoconf and "unnecessarily" enforces or hardcodes certain compiler or linker flags the ebuild usually strips these away (either on-the-fly as part of the ebuild (via sed or an appropriate eclass) or via a separate patch file). If we don't have this at the moment, we should allow overriding C[XX]FLAGS via the commandline. As long as that is possible I see no problem defaulting to -Og.
There was a problem hiding this comment.
Let me clarify: I see no problem defaulting to -Og for debug builds, just as bgK says.
There was a problem hiding this comment.
OK, I understand now, thank you for clarifying. So is the best approach then to not do this at all without an opt-in, not do this if CXXFLAGS is set, try to test for an -O flag in CXXFLAGS and not do it if one is found, something else?
There was a problem hiding this comment.
I'm very unsure what's the best option. We seem to enable debug builds by default. Perhaps when --disable-debug is passed to configure then -Og should not be set. That way it's possible not to have optimization flags without editing configure.
There was a problem hiding this comment.
Oh, but it's also possible to pass --disable-optimizations to disable -Og. Then I guess it's good.
There was a problem hiding this comment.
Sounds good, we can refine this later.
| ;; | ||
| --disable-c++11) | ||
| _use_cxx11=no | ||
| --disable-c++11|--force-c++98) |
There was a problem hiding this comment.
I'm not sure I like the idea of adding an option to configure to support a C++ standard version that will likely no longer be accepted in the near future.
What do you think about adding something like --with-c++-dialect=c++98 instead?
There was a problem hiding this comment.
No sure it's better actually because then we need to add checks for the allowed version...
There was a problem hiding this comment.
I don’t have any strong preference either way. I guess the arguments for keeping it this way instead of using something like --with-c++-dialect= are that (1) it has a clearly identifiable end-of-useful-life where it can be removed, and (2) it doesn’t offer any easy avenue for users to attempt use of incompatible language standards.
There was a problem hiding this comment.
Personally I'd prefer --with-c++-dialect=foo but I'm fine with either one.
|
Any updates on this PR? |
Instead of hard-coding these lists into the CI system's packaging code, expose them from Make so that everything is sourced off the same one list.
C++ sqrt is overloaded so operates using single-precision when receiving a float input. The C standard library on FreeMiNT does not fully support C99 math so use of sqrtf instead of sqrt(float) does not work.
C++ math functions are overloaded so operate using single-precision when receiving a float input. The C standard library on FreeMiNT does not fully support C99 math so use of sqrtf, sinf, etc. instead of the C++ API does not work.
strdup is a POSIX API, not an ANSI C API. It is not available with -std=c++11 on newlib-based systems, and VS 2015 will throw errors unless it is #defined to alias to _strdup or unless deprecation warnings are turned off (which is not a good idea in general). Common::String is a safer and potentially faster (due to small string optimisation) alternative, so prefer it instead.
It was also necessary to make sure that the Common::String objects were initialised correctly by switching to use a C++ container for engine objects instead of calloc, since they were no longer C-compatible PODs.
Also removes an unnecessary second condition check for oldAnswer by moving that closer to its point of use.
Parallaction::_gfx is referenced by objects destroyed when Parallaction::_input is destroyed so it cannot be destroyed first.
Debian has an armhf cross-compiler. If users need to change the location of files they can set up the environment flags correctly themselves.
csnover
force-pushed
the
buildbot
branch
3 times, most recently
from
3344372
to
18d7cf3
Compare
Eventually this should probably also use -static or -Wl,-Bstatic for non-Darwin platforms. For now, it only does these things: 1. Switches to use --static-libs/--static when getting dependencies from the third-party libraries with configuration scripts, since this is needed to get the correct -framework flags from SDL2 and extra dependencies from FreeType2; 2. Rewrites linker flags from -lfoo to $staticlibpath/lib/libfoo.a, since this is required in order to get the Apple linker to do static linking whenever there is a shared library available. This commit changes the recently added --enable-static flag name since that flag DOES NOT actually generate static builds, it only changes which dependencies are requested from third party library configuration scripts.
The local documents are not currently internationalised simply because the internationalised resources are not put into the right places (NSBundle will handle this automatically when they are); Trac#10464 is a tracking bug for this outstanding issue. Also, in order to reduce the complexity of showing the licensing information, the licensing files are now collapsed into a single COPYING file, and non-license information (like the instructions from GNU on how to apply the GPL to a software project) has been stripped. This is of benefit to everyone, since some ports were not including all the correct licensing information since they weren’t updated when new license files were added to the codebase. Fixes Trac#10437.
|
I added the Help menu for macOS. It doesn’t include selection of the appropriate internationalised readme file at the moment; I added a new ticket for tracking that separately with some information, since I will not be able to do that work. The new Help menu includes a link to the user manual which does not exist currently, https://www.scummvm.org/manual, which should just redirect to the wiki. I did it this way because that link should always work into the future regardless of where the user manual might end up, and because it’s 2018 so I don’t want to add non-HTTPS URLs to the binary. To avoid having to have 3 different menu items for licenses, and to stop any future license violations in ports caused by not including all the licenses, I also collapsed all the license files into a single COPYING file. The other ports’ file lists should be adjusted appropriately. I added code in So, I think that is all the critical feedback addressed. Let me know if not. I would like to be able to land this now so this can be finally done, but I never received the access that I requested to move the other repositories into the organisation and to make sure it is possible to recover if there is a contingency during the required server updates. As such, there is nothing left for me to do here, and someone else with access will need to do that I guess. |
There was a problem hiding this comment.
I added few comments. Still not ready to merge.
And of course, it got rot due to the active development.
It is not clear to me why some functionality is dropped. Also, few changes like switching to C++14 as default need to be discussed with the porters first.
Most of the engine-related changes like removal of strdup and sqrtf are worthy adding sooner, so I'd suggest to split them out to the separate PR.
| @@ -55,7 +55,7 @@ patent must be licensed for everyone's free use or not licensed at all. | |||
|
|
|||
| The precise terms and conditions for copying, distribution and | |||
| modification follow. | |||
|
|
|||
There was a problem hiding this comment.
I am not happy with all licenses lumped into a single file. There are several reasons for this: 1. Not whole ScumVM code base is equally licensed, thus, there are disclaimers on top of relevant license files. 2. The lumped file will make it more difficult to understand which licenses are used.
If you need a combined file in the distribution, this could easily be generated, but we still would better ship the combined version along with the split ones.
| #define FORBIDDEN_SYMBOL_EXCEPTION_mkdir | ||
| #define FORBIDDEN_SYMBOL_EXCEPTION_exit //Needed for IRIX's unistd.h | ||
| // Re-enable forbidden symbols to avoid clashes with stat.h and unistd.h. | ||
| #define FORBIDDEN_SYMBOL_ALLOW_ALL |
There was a problem hiding this comment.
What was the problem? We always wanted to specify the exceptions explicitly in order to detect compatibility issues earlier. Enabling everything voids purpose of this directive.
| #define FORBIDDEN_SYMBOL_EXCEPTION_getenv | ||
| #define FORBIDDEN_SYMBOL_EXCEPTION_exit //Needed for IRIX's unistd.h | ||
| // Re-enable forbidden symbols to avoid clashes with stat.h and unistd.h. | ||
| #define FORBIDDEN_SYMBOL_ALLOW_ALL |
|
|
||
| _videoMode.screenWidth = w; | ||
| _videoMode.screenHeight = h; | ||
| if (w > 320 || h > 240) { |
There was a problem hiding this comment.
And why this functionality is now being killed?
| @@ -60,11 +60,6 @@ bool ModularBackend::getFeatureState(Feature f) { | |||
| return _graphicsManager->getFeatureState(f); | |||
| } | |||
|
|
|||
| GraphicsManager *ModularBackend::getGraphicsManager() { | |||
There was a problem hiding this comment.
Why was this moved to SDL backend only? There are other backends besides SDL.
| @@ -2843,20 +2901,11 @@ if test -n "$_host"; then | |||
| arm-*riscos) | |||
| _opengl_mode=none | |||
| _build_hq_scalers=no | |||
| # toolchain binaries prefixed by host | |||
There was a problem hiding this comment.
Is the cross-compilation still functioning with this patch?
| # but Raspbian does. | ||
| # Be careful as it's the linker (LDFLAGS) which must know about sysroot. | ||
| # These are needed to build against Raspbian's libSDL. | ||
| append_var LDFLAGS "--sysroot=$RPI_ROOT" |
There was a problem hiding this comment.
Was the build system tested for raspberry pi?
| # Enable optimizations. This also | ||
| # makes it possible to use -Wuninitialized, so let's do that. | ||
| if test -z "$_optimization_level" ; then | ||
| # For pretty much all systems, -Os is the best level if it is supported |
There was a problem hiding this comment.
And what if a porter has a better idea than the compiler default? We should not remove the possibility to specify it explicitly.
|
|
||
| # Special target to create a snapshot disk image for Mac OS X | ||
| # TODO: Replace AUTHORS by Credits.rtf | ||
| osxsnap: bundle |
There was a problem hiding this comment.
I am also not sure why we are suddenly dropping this.
| @@ -450,7 +279,7 @@ scummvmwinres.o: $(srcdir)/icons/scummvm.ico $(DIST_FILES_THEMES) $(DIST_FILES_N | |||
| $(QUIET_WINDRES)$(WINDRES) -DHAVE_CONFIG_H $(WINDRESFLAGS) $(DEFINES) -I. -I$(srcdir) $(srcdir)/dists/scummvm.rc scummvmwinres.o | |||
|
|
|||
| # Special target to create a win32 snapshot binary (for Inno Setup) | |||
| win32dist: $(EXECUTABLE) | |||
| win32dist: all | |||
There was a problem hiding this comment.
Does it mean that it builds the devtools too?
|
I'm closing this since @ccawley2011 split it into a number of PRs, a big chunk of which has just been merged. This is an outstanding work, and it's a pity to see it go with Colin's departure. |
This PR contains changes to work with the new Buildbot service,
which is currently at https://github.com/csnover/scummvm-buildbot/,
and to modernise small portions of the build system.
More information about the new Buildbot service can be found in the
[http://lists.scummvm.org/pipermail/scummvm-devel/2017-December/012041.html](mailing list post).
Here is a summary of the most notable changes:
do not exist in standard C++, like strdup and sqrtf, have been
updated to replace these non-standard APIs with standard ones.
This is necessary because newer platform compilers with stricter
ANSI C++ compliance fail to compile this code.
optimisations enabled, the default is -Os with auto-vectorisation,
and without, -Og. Feature detection is used to fall back to older
optimisation modes -O2 and -O0 if these options are not
available.
to improve linker performance.
removed everywhere since the new defaults should be appropriate
for all platforms[1].
--enable-optimizationshas been enhancedto accept an optional flag, so users can choose to force an
optimisation mode that way if needed.
has been added, so the latest supported standard C++ version is
now used by default instead of defaulting to C++98/03 and
requiring users to opt in to the newer standards modes. The
--enable-c++11flag is now a no-op and--force-c++98can beused to force compilation using the legacy standard.
--enable-static-libsflag is added to use--static-libsinstead of
--libswhen collecting linker information fromthird-party scripts. This is needed for macOS/iOS. Eventually
this should be expanded to actually link libraries statically
when enabled, so static builds can be generated even in build
environments which also have shared libraries, but this is not
needed right now for Buildbot so is not fully implemented. (This
would be most useful for the Linux builds, which right now will
only work against particular versions of Debian due to dynamic
linking.)
removed, and non-standard system/library APIs have been fixed to
use the correct APIs.
compiler overrides (e.g. STRINGS, STRIP, etc.).
in lieu of the normal feature detection sytem.
macOS static builds. (The macOS static builds situation is still
not ideal, but it does function well enough in conjunction with
the Buildbot image to generate working macOS builds.)
The engines with code changes are AGI, Bladerunner, Director, Gob,
Lure, Parallaction, SCUMM, Sword2. I did some basic testing of all
the engines except Bladerunner to try to check that these code
changes haven’t broken anything obviously, but as always
additional review/testing on these changes is welcome/encouraged.
The ports with changes are 3DS, AmigaOS, Android, Caanoo, Dingux,
Dreamcast, FreeMiNT, GameCube, GCW0, iOS, macOS, Maemo, Open
Pandora, PSP, Raspberry Pi, RISC OS, Wii, Win32, WinCE. (Most of
these changes are just the optimisation mode override removals.)
Builds from the new Buildbot were tested over a couple-week period
a few weeks ago to verify that they do at least run on these
platforms, which cover >98% of our users by downloads:
I will leave it up to others to test/fix any of the other ports.
[1]: Vita doesn’t have enough memory to load a binary with all
engines statically compiled without -Os, so the default
non-optimised build doesn’t work. rsn8887 is aware of this and
is aware that the Vita port needs dynamic plugin support (since
eventually -Os won’t be able to remove enough code to fit in
memory either). For consistency, and to ensure we are getting
more exercise of the dynamic plugins system, we should probably
start enabling plugins by default at this point for all
platforms, but that can be changed & discussed separately.