0003905: zandronumbinaries.cpp OS-dependent macro hell is quite ugly (and broken)

MantisBT - Doomseeker

View Issue Details

Project

Category

View Status

Date Submitted

Last Update

0003905

Doomseeker

[All Projects] Cleanup

public

2021-09-24 15:40

2021-09-28 23:29

Reporter

WubTheCaptain

Assigned To

Priority

low

Severity

minor

Reproducibility

have not tried

Status

confirmed

Resolution

open

Platform

OS Version

Product Version

1.3.2

Target Version

Fixed in Version

Summary

0003905: zandronumbinaries.cpp OS-dependent macro hell is quite ugly (and broken)

Description

This macro hell was too cumbersome to read for me:

Quote from src/plugins/zandronum/zandronumbinaries.cpp

#ifdef Q_OS_WIN32
#define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "windows.zip"
#define ZANDRONUM_BINARY_NAME "zandronum.exe"
#define SCRIPT_FILE_EXTENSION ".bat"
#else

#ifndef __x86_64__
#define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "linux-x86.tar.bz2"
#else
#define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "linux-x86_64.tar.bz2"
#endif

#ifndef Q_OS_DARWIN
#define ZANDRONUM_BINARY_NAME "zandronum"
#else
#define ZANDRONUM_BINARY_NAME "Zandronum.app"
#define ZANDRONUM_APP_BUNDLE_BIN "/Contents/MacOS/zandronum"
#endif
#define SCRIPT_FILE_EXTENSION ".sh"
#endif

The first Q_OS_WIN32 seems fine. Then the latter confused me.

I forgot it's not possible to re-define a macro without undefining it first in C++. This initially led me to believe 64-bit Windows would download GNU/Linux binaries, and use a binary named zandronum instead of zandronum.exe.
After copying the code to my text editor and adding spaces, I figured the first Q_OS_WIN32 block doesn't even end in #endif and changed my understanding of the code entirely.

With spaces and newlines, the above code is the same as:

#ifdef Q_OS_WIN32
    #define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "windows.zip"
    #define ZANDRONUM_BINARY_NAME "zandronum.exe"
    #define SCRIPT_FILE_EXTENSION ".bat"
#else
    #ifndef __x86_64__
        #define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "linux-x86.tar.bz2"
    #else
        #define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "linux-x86_64.tar.bz2"
    #endif

    #ifndef Q_OS_DARWIN
        #define ZANDRONUM_BINARY_NAME "zandronum"
    #else
        #define ZANDRONUM_BINARY_NAME "Zandronum.app"
        #define ZANDRONUM_APP_BUNDLE_BIN "/Contents/MacOS/zandronum"
    #endif

    #define SCRIPT_FILE_EXTENSION ".sh"
#endif

Suddenly more understandable (thanks, spaces!), yet still ugly (and broken in some ways).

Other issues here, some which may be seperate issues:

__x86_64__ probably isn't portable. Qt handily has Q_PROCESSOR_X86_64 (at compile-time), which I found to be slightly more portable (and not mostly specific to GCC).
Anything non-Windows (except Q_OS_DARWIN, which has a special case later) will still download GNU/Linux binaries (because of the current state of the Zandronum downloads web directory). Hello OpenBSD.
Zandronum 3.1 (210919-1948) switched its downloadable 64-bit binary distribution filenames to linux-x64.tgz.
Q_OS_DARWIN includes iOS and stuff, so perhaps Q_OS_MACOS would be more appropriate (based on the comment).
I understand the current state allows the user to configure the exact file for Zandronum stable binaries, yet testing binaries allow the user to configure the testing directory (where we've hard-coded Zandronum testing binary filenames for scripts to be created). I would sort of like this split abstraction to cease existing for code maintainability to reduce the amount of preprocessor macros and inflexibility, although it would likely mean losing support for multiple testing versions of Zandronum without user configuration. (I have thought of alternative solutions to the problem: symlinks or a "firefox.real" approach taken by Debian.)

Steps To Reproduce

Additional Information

src/plugins/zandronumq/zandronumqbinaries.cpp does not have the __x64_64__ macro, no support for GNU/Linux with that plugin I assume.

Tags

No tags attached.

Relationships

related to	0003906	confirmed		Doomseeker on OpenBSD may download Zandronum's GNU/Linux testing binaries
related to	0003908	acknowledged		zandronumbinaries.cpp does not make sure the downloaded Zandronum testing binaries are supported by the CPU architecture

Attached Files

Issue History

Date Modified

Username

Field

Change

2021-09-24 15:40