Menu

#314 linux amd64, BUGBUG: scons 'TARGET_ARCH' should specify the default

3.x Stable
open
nobody
None
5
2024-04-27
2024-04-21
No
diff --git a/Source/SConscript b/Source/SConscript
index dbf4ba9..f8b223d 100644
--- a/Source/SConscript
+++ b/Source/SConscript
@@ -77,6 +77,8 @@ AddZLib(env, env['PLATFORM'], 'install-compiler')

 env.Append(CPPDEFINES = ['MAKENSIS'])
 env.Append(CPPDEFINES = ['_WIN32_IE=0x0500'])
+TARGET_ARCH_enum_dict ={'x86':'TARGET_X86UNICODE', 'amd64':'TARGET_AMD64', 'arm64':'TARGET_ARM64'}
+env.Append(CPPDEFINES = [f'TARGET_ARCH_ENUM={TARGET_ARCH_enum_dict[env['TARGET_ARCH']]}'])

 ##### Set PCH

diff --git a/Source/build.cpp b/Source/build.cpp
index 6f0a986..6fb6652 100644
--- a/Source/build.cpp
+++ b/Source/build.cpp
@@ -129,14 +129,8 @@ CEXEBuild::CEXEBuild(signed char pponly, bool warnaserror) :
   definedlist.add(_T("NSIS_VERSION"), NSIS_VERSION);
   definedlist.add(_T("NSIS_PACKEDVERSION"), NSIS_PACKEDVERSION);

-  m_target_type=TARGET_X86UNICODE;
-#ifdef _WIN32
-  if (sizeof(void*) > 4) m_target_type = TARGET_AMD64; // BUGBUG: scons 'TARGET_ARCH' should specify the default
-#endif
-#ifdef _M_ARM64
-  m_target_type = TARGET_ARM64; // BUGBUG: scons 'TARGET_ARCH' should specify the default
-#endif
-  build_unicode=TARGET_X86ANSI != m_target_type;
+  m_target_type=TARGET_ARCH_ENUM;
+  build_unicode=true;
   build_lockedunicodetarget=false;

   // automatically generated header file containing all defines

under linux, m_target_type will default to TARGET_X86UNICODE, even when TARGET_ARCH=amd64, this causes it to search for Stubs/zlib-x86-unicode, when Stubs/zlib-amd64-unicode is what exists, then this will be thrown:
throw runtime_error("error setting default stub");

reason I put build_unicode=true;: build_unicode will always be true based on the above code: build_unicode is initialized to TARGET_X86UNICODE or set to something else

Discussion

  • Jason

    Jason - 2024-04-22

    Thanks. We haven't decided on how we will build and release 64bit versions of NSIS yet, so creating 64 bit installers is unofficial. Most linux-like package managers don't have this issue, because they usually cross-compile it twice for x86 and amd64, and the user has to specify it in the script to use versions other than 32bit.

    I think the main issue is how we set the default for cross compiled targets like this, particularly on linux-like hosts where the compilation order would dictate which target would be the default. NSIS is somewhat unique in this regard, as we compile the source code not only for the host, but for the target machines too.

    I remember years ago we talked about this while I helped fix the posix build for NSIS 3, but as usual, compiling on windows hosts is a bit of a pain because of backwards compatibility. I think the main point was to have a 32bit based install and figure out how to compile the 64bit stuff and include in the 32bit version. And who knows how many people use "windows on ARM", that's another kettle of fish altogether (that version isn't even finished yet).

    The easiest workaround at the moment is to create a dummy stub for 'Stubs/zlib-x86-unicode', and then in the script use 'Target amd64-unicode'.

     
  • Fu Pei Jiang

    Fu Pei Jiang - 2024-04-22

    this fixes the case where only amd64 is targetted, and it doesn't break anything else, or does it ?


    particularly on linux-like hosts where the compilation order would dictate which target would be the default.

    I don't think this is true, when building for a linux host, _WIN32 is not defined, and thus m_target_type will always be TARGET_X86UNICODE, the target is constant and not dependent on compilation order.
    Unless you mean after this patch is merged, then it would be true.


    https://stackoverflow.com/questions/152016/detecting-cpu-architecture-compile-time#66249936
    this would be more correct

    #if defined(__x86_64__) || defined(_M_X64)
      m_target_type = TARGET_AMD64;
    #elif defined(i386) || defined(__i386__) || defined(__i386) || defined(_M_IX86)
      m_target_type = TARGET_X86UNICODE;
    #elif defined(__aarch64__) || defined(_M_ARM64)
      m_target_type = TARGET_ARM64;
    #else
      #error Unsupported target architecture
    #endif
    

    but I don't get what the above logic is trying to do,
    it is defining the target machine architecture based on the host machine architecture.
    All of these macros should not be used, since they are of the host architecture, while we are setting a variable for target architecture
    I think the only correct way to do this would be reading a macro based on target architecture, since none of these macros exist, we have to create one, it can be based on TARGET_ARCH passed to scons

     
  • Jason

    Jason - 2024-04-23

    Well, in my fork (NSISBI), I did the opposite. I forced the target to always be x86-unicode by default, compiled two times with VS2008 or (more recently) VS2005 to get both x86 and amd64, and I only provide a zipped portable version of the binary package because I don't want to deal with modifying the build system and setup .nsi to make an installer binary too.

    Unfortunately, linux-like systems are the minority here, despite the fact that it's easier to setup, build and deploy on it. The vast majority of downloads are the windows installer binaries, so getting those to work is the priority I think.

    I think the best choice here is to make the compiler default x86-unicode, and let the script writer choose if they want to target amd64 instead. On linux-like platforms this is much easier to support because you don't need a dedicated installer, the build system is the installer, so you just compile it twice for both targets. Trying to do this on windows is where things hit the fan, because windows users expect a windows installer to be available. And because our release system for windows is automated, we would have to figure out how to automate compiling amd64 binaries and somehow including it in the windows installer. As well as creating the logging build and the strlen_8192 build and the portable build too.

    What else can I say, no decision has been made yet; and it's alot of work that noone has stepped up to do. At least on my fork, I have somewhat experimented a bit with including both targets.

     
  • Fu Pei Jiang

    Fu Pei Jiang - 2024-04-24

    I think you are misunderstanding,

    • build machine is the computer that is doing the actual compiling.
    • host machine is the machine on which the compiled binary will run.
    • target machine is the machine on which the compiled binary's output will run, only meaningful if the program produces machine-specific output.

    you are cross compiling even on windows, the host is 32bit, the target is 64bit
    I'm not asking for a 64bit host, I'm asking for a 64bit target
    the strlen_8192 build, logging build, and portable build can all remain 32bit


    right now, a 32bit windows host will always default to 32bit target
    a 64bit windows host will always default to 64bit target
    what I want is for a 32bit windows host to default to 64bit target

    but all of these options are bad, because now the same command line will produce different outputs, depending on how the host was compiled, so I can half agree that the target should ALWAYS default to x86-unicode.

      m_target_type=TARGET_X86UNICODE;
    #ifdef _M_ARM64
      m_target_type = TARGET_ARM64; // BUGBUG: scons 'TARGET_ARCH' should specify the default
    #endif
    

    this is from your fork (NSISBI); target should not default to arm64-unicode if the host is arm64, all that should remain in the code is m_target_type=TARGET_X86UNICODE;


    in order to prevent command line output ambiguity, the target should always be x86-unicode,
    then the error reporting should be fixed, if we want to target amd64-unicode, but we didn't compile the stub for x86-unicode: the error should be reported when m_target_type is finally needed

      if (set_compressor(_T("zlib"), false) != PS_OK || set_target_architecture_data() != PS_OK)
      {
        throw runtime_error("error setting default stub");
      }
    

    this causes an unneeded read and loading of the default stub; only the name should be default, not the loading, I'll see what's the latest point before which the stub must be loaded, I know at least it's: "[before] data already got compressed"

     
    • Anders

      Anders - 2024-04-27

      what I want is for a 32bit windows host to default to 64bit target

      Then you should just say so in your .nsi

       
  • Jason

    Jason - 2024-04-24

    Regarding ARM64 support, we did try compiling for ARM64, but the linker is missing the "/FIXED" flag (fixed base address) that we require for the installer to run. So unless we find a workaround for this, it's basically a dead end. I tested this several years ago with windows on a Raspberry Pi, most of the NSIS tools work when the runtime redist is installed, but the installers themselves don't.

    I understand that 64bit installers would be nice to have, but nsis can already install a 64 bit app using some helpers, even though the installer is 32bit itself. And anyway, x86-unicode installers run just fine on ARM64 (with a few script helpers) using a compatibility layer built into windows.

    I know it's a bit weird how we set the build system up initially, but if you are compiling code anyway, just compile it the way you need it. In 99% of cases, our advice is to compile NSIS twice and take the bits you need from both to make a hybrid install, just like I do with NSISBI (in fact, recently I've been doing a third compile using VC6 for the x86-ansi stubs for my binary release too so that windows 9x installers still work).

    The bottom line is: unless 32bit support is entirely removed from windows, there just isn't any incentive to change what already works.

     
  • Fu Pei Jiang

    Fu Pei Jiang - 2024-04-24

    automate compiling amd64 binaries and somehow including it in the windows installer. As well as creating the logging build and the strlen_8192 build and the portable build too.

    ok, I finally understood: currently the amd64 stubs aren't included in any of these distributions
    only in NSISBI and linux package manager are the amd64 stubs included,

    I'll clone the repo and make a .github\*.yml, is that the correct way to contribute ?
    am I correct to asssume that github actions is the one building the releases ?

    with such a *.yml, what else would there be to consider ?
    is it even desirable to support 64bit installers ?
    it would increase the size of the .zip for instance

    https://github.com/kichik/nsis/actions/runs/8497460576
    doesn't tell me how nsis-3.10-strlen_8192.zip is built, its contents are not found in the artifact build.zip

    where is the actual building being done ?

     
  • Jason

    Jason - 2024-04-26

    If you have a look in 'Scripts\release.py' you'll see that is how we deploy a release on sourceforge.

    Couple of things to note:
    1: I don't know the computer setup used to compile these releases, but there is a section in the help documents specifically for setting up tools to compile the code
    2: The "system" plugin needs it's amd64 assembly file fully implemented, callbacks are not currently implemented for 64 bit installers using this plugin (hence why it's unofficial)
    3: I think it would be a good idea to split off the "dist" part of the build system in scons into a separate script or scripts so that it's easier to combine a release binary together with different architectures.
    4: The strlen_8192 and logging builds are actually rather easy to produce an additional amd64 version alongside the x86 release, as long as an appropriate x64 compiler is available (my choice is VS2005)
    5: I think keeping all the base tools as x86 is a good idea for compatibility, and having amd64 only as installer targets would make the most sense

    One of the official devs can chime in as well to point us in the right direction (I'm just a regular contributor on the posix side of development).

     
  • Fu Pei Jiang

    Fu Pei Jiang - 2024-04-26

    thanks for 'Scripts\release.py', I think I'll make a github workflow, I feel like it would be more automated, I'll takes notes from the .py

    2: The "system" plugin needs it's amd64 assembly file fully implemented, callbacks are not currently implemented for 64 bit installers using this plugin (hence why it's unofficial)

    I'll try to make the amd64 version, 900lines of difference, (you'll probably never hear from me by the time I finish)

    (hence why it's unofficial)

    is there anything else ?

    3: I think it would be a good idea to split off the "dist" part of the build system in scons into a separate script or scripts so that it's easier to combine a release binary together with different architectures.

    what about turning TARGET_ARCH into a comma separated list ?

     
  • Jason

    Jason - 2024-04-27

    Maybe. This is the main issue with the build system, it's only setup for one architecture at a time. Back when the build system was built in 2005, 64 bit compilers weren't really a thing yet, so it was setup as one architecture at a time. ( I remember this quite well, because I first got involved in NSIS right before this new build system came in).

    The idea is to call the build system twice with different parameters, and have the distribution assembled separate from the build system. Think of it like how Visual Studio has organized its compilers: the IDE might be 32bit, and the compilers and linkers might be 32bit native and target other architectures as an output.

    NSIS is mostly in the same boat in this regard, we aren't just compiling tools, we also compile target architectures that the tools use, at the same time. This double layer of targeting is why it's trickier to build than a single app.

     

Log in to post a comment.