Zandronum Chat on our Discord Server Get the latest version: 3.2
Source Code
My View | View Issues | Change Log | Roadmap | Doomseeker Issue Support Ranking | Rules | My Account
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0003905Doomseeker[All Projects] Cleanuppublic2021-09-24 15:402021-09-28 23:29
ReporterWubTheCaptain 
Assigned To 
PrioritylowSeverityminorReproducibilityhave not tried
StatusconfirmedResolutionopen 
PlatformOSOS Version
Product Version1.3.2 
Target VersionFixed in Version 
Summary0003905: zandronumbinaries.cpp OS-dependent macro hell is quite ugly (and broken)
DescriptionThis 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.)
Additional Informationsrc/plugins/zandronumq/zandronumqbinaries.cpp does not have the __x64_64__ macro, no support for GNU/Linux with that plugin I assume.
Attached Files

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

-  Notes
User avatar (0021761)
WubTheCaptain (reporter)
2021-09-24 15:46
edited on: 2021-09-24 17:54

 Nevermind I did not do the "if Windows" / "if not Windows" split here, for each OS the first part could look like:

#ifdef Q_OS_LINUX
    #ifndef Q_PROCESSOR_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

    #define ZANDRONUM_BINARY_NAME "zandronum"
    #define SCRIPT_FILE_EXTENSION ".sh"
#endif

#ifdef Q_OS_MACOS
    #define ZANDRONUM_BINARY_NAME "Zandronum.app"
    #define ZANDRONUM_APP_BUNDLE_BIN "/Contents/MacOS/zandronum"
    #define SCRIPT_FILE_EXTENSION ".sh"
#endif

#ifdef Q_OS_WIN32
    #ifndef Q_OS_WIN64
        #define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "windows.zip"
    #else
        #define TESTING_BINARY_URL TESTING_BINARY_URL_BASE "windows-x64.7z"
    #endif

    #define ZANDRONUM_BINARY_NAME "zandronum.exe"
    #define SCRIPT_FILE_EXTENSION ".bat"
#endif


It's not as concise and duplicates SCRIPT_FILE_EXTENSION once (can be macroed again to "if Windows" / "if not Windows"), but it's immediately mostly sensible.
User avatar (0021765)
WubTheCaptain (reporter)
2021-09-24 16:28
Quote from WubTheCaptain
  • 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.

This is also issue 0003906.
User avatar (0021769)
WubTheCaptain (reporter)
2021-09-24 17:12
edited on: 2021-09-24 17:14
Quote from WubTheCaptain
  • __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).

I had deleted my earlier note here, but the imprecision also implied in OP and incorrectly in the above example is that GNU/Linux on non-x86 processors (e.g. ARM) will still download the x86 binaries, instead of refusing.
User avatar (0021779)
Pol M (developer)
2021-09-28 23:29

 All the points are logical and make sense, seems good to me

Issue Community Support
Only registered users can voice their support. Click here to register, or here to log in.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2021-09-24 15:40 WubTheCaptain New Issue
2021-09-24 15:46 WubTheCaptain Note Added: 0021761
2021-09-24 15:59 WubTheCaptain Note Added: 0021762
2021-09-24 15:59 WubTheCaptain Note Edited: 0021762 View Revisions
2021-09-24 16:00 WubTheCaptain Note Deleted: 0021762
2021-09-24 16:28 WubTheCaptain Note Added: 0021765
2021-09-24 16:28 WubTheCaptain Relationship added related to 0003906
2021-09-24 17:12 WubTheCaptain Note Added: 0021769
2021-09-24 17:12 WubTheCaptain Note Edited: 0021769 View Revisions
2021-09-24 17:14 WubTheCaptain Note Edited: 0021769 View Revisions
2021-09-24 17:35 WubTheCaptain Note Added: 0021772
2021-09-24 17:35 WubTheCaptain Note Deleted: 0021772
2021-09-24 17:36 WubTheCaptain Relationship added related to 0003908
2021-09-24 17:54 WubTheCaptain Note Edited: 0021761 View Revisions
2021-09-24 18:06 WubTheCaptain Description Updated View Revisions
2021-09-28 23:29 Pol M Note Added: 0021779
2021-09-28 23:29 Pol M Status new => confirmed




Questions or other issues? Contact Us.

Links

Copyright © 2000 - 2025 MantisBT Team
Powered by Mantis Bugtracker