Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MacOS version #86

Open
digitalbox94 opened this issue Aug 9, 2024 · 39 comments
Open

MacOS version #86

digitalbox94 opened this issue Aug 9, 2024 · 39 comments

Comments

@digitalbox94
Copy link

MacOS version is compiling fine with 2 adjustments : are PR accepted to fix such issue ?

-TEMPLATE = vcapp
+TEMPLATE = app

-CONFIG += c++20
+CONFIG += c++17

MacOS
@gavrant
Copy link

gavrant commented Aug 10, 2024

I wonder what is the problem with C++ 20 on MacOS? Is it the compiler of your choice yet to support that standard? Or is it something else?

@digitalbox94
Copy link
Author

When using c++20, I have the below error :
src/gl/glmesh.cpp:276:23: error: no member named 'make_unique' in namespace 'std'
auto tempMdl = std::make_unique( this );

Checking the compiler version, I can see this version :
Apple clang version 15.0.0 (clang-1500.3.9.4)

@gavrant
Copy link

gavrant commented Aug 11, 2024

15.0.0 looks pretty outdated for LLVM clang, the latest version is 18.1.8. Though, to be fair, I know nothing about the difference between "Apple clang" and "vanilla (LLVM) clang". I just think that "fixing" clang is a better option here than going back to C++ 17.

@digitalbox94
Copy link
Author

15.0.0 looks pretty outdated for LLVM clang, the latest version is 18.1.8. Though, to be fair, I know nothing about the difference between "Apple clang" and "vanilla (LLVM) clang". I just think that "fixing" clang is a better option here than going back to C++ 17.

Ok I've updated clang with homebrew and it's now with version 18.1.8

I've added these lines in the pro file :
QMAKE_CC = /usr/local/opt/llvm/bin/clang
QMAKE_CXX = /usr/local/opt/llvm/bin/clang++
INCLUDEPATH += /usr/local/opt/llvm/include

However qmake is applying automatically some defaulting on the "std" options as per below :
CONFIG += c++20 => -std=gnu++11 => error is still there on MacOS
CONFIG += c++17 => -std=gnu++1z => OK on MacOS

Not sure what this mapping of the CONFIG means and if it's correct, but for the moment II need to use c++17

@DigitalBox98
Copy link

Oh and I’m using qt 5.12.x so maybe that’s why this mapping is not good ? (I need to check how the update to 5.15 can be done )

@gavrant
Copy link

gavrant commented Aug 11, 2024

CONFIG += c++20 => -std=gnu++11 => error is still there on MacOS
CONFIG += c++17 => -std=gnu++1z => OK on MacOS

I’m using qt 5.12.x

Oh, I think I know what's going on! My guess is: if qmake (the Qt tool you used to create a clang makefile from NifSkope.pro) is that outdated, it doesn't know about C++20, considers it a bad option value and defaults the makefile to C++11. So yes, updating Qt looks like the way to go.

@DigitalBox98
Copy link

It makes sense, I will try to update to 5.15 to have the good qmake version :)

Thanks !

@digitalbox94
Copy link
Author

Ok I've upgraded to QT 5.15 with latest qmake and I've got this version compiling & running fine on MacOS latest version.

However I don't know why but c++20 is not recognized and instead I have to use c++2a for the CONFIG. Plus a few additional adjustements below :

Below are the changes for the pro file :
TEMPLATE = app
QMAKE_CC = /usr/local/opt/llvm/bin/clang
QMAKE_CXX = /usr/local/opt/llvm/bin/clang++
INCLUDEPATH += /usr/local/opt/llvm/include
CONFIG += c++2a
QMAKE_CFLAGS = -fno-define-target-os-macros
ICON = nifskope.icns # addition of an icon for MacOS

Below are the changes for the NifSkope_targets.pri :
f(!isEmpty(dot)) {
exists($$dot) {
HAVE_DOT = YES
DOT_PATH = $$re_escape($${dot})
}
}

Can I submit a PR for that ?

With macdeployqt I've been able to produce also a DMG image for Nifskope (xml files must be copied into the NifSkope.app/Contents/MacOS folder before)

app

@gavrant
Copy link

gavrant commented Aug 15, 2024

What version of Qt 5.15 you upgraded to? In your compiled NifSkope, what version is reported in "About Qt" from Help menu? That "c++2a" makes me think that your Qt is still 2 or more years old.

If you submit your changes as PR, will it break NifSkope.pro for all other systems and compilers other than LLVM clang on MacOS? I'm asking because:

  • TEMPLATE = app - won't work for Microsoft C++ (Visual Studio) on Windows. vcapp is in the current NifSkope.pro exactly because of VS.
  • The vars below are hardcoded to the clang paths on your system. Shouldn't they be outside of the .pro file, in your local bash script instead?
    QMAKE_CC = /usr/local/opt/llvm/bin/clang
    QMAKE_CXX = /usr/local/opt/llvm/bin/clang++
    INCLUDEPATH += /usr/local/opt/llvm/include
  • CONFIG += c++2a - I still believe that "c++20" should remain there because I've been compiling NifSkope with it for a year (on Windows though) without issues.
  • QMAKE_CFLAGS = -fno-define-target-os-macros - is it connected to zlib not compiling on MacOS?
  • ICON = nifskope.icns - another thing exclusive to MacOS?

Regarding NifSkope_targets.pri and the "dot" thing - just skip building the whole doxygen part if you can. NifSkope is functional without it.

@fo76utils
Copy link

fo76utils commented Aug 15, 2024

Can I submit a PR for that ?

For what it is worth, I merged your changes to my fork here, although I cannot test them, because I only have Linux and Windows PCs. I did make a few modifications:

  • I set TEMPLATE = vcapp only for MSVC, and 'app' for everything else.
  • Removed the -fno-define-target-os-macros workaround, since I do not use zlib.
  • Moved the icon file to res/.
  • I also had problems with the same part of NifSkope_targets.pri in the past, so I already changed that code, but in a different way.

@gavrant
Copy link

gavrant commented Aug 16, 2024

I adjusted the default value of TEMPLATE in my fork too.

That fno-define-target-os-macros is connected to zlib is my guess based on "cmake doesn't compile with clang 18 due to deprecated macros in zutil.c and gzguts.h" being the first result if I search for "fno-define-target-os-macros" on Google. And that thread promises that this problem is fixed in cmake 3.30. Can't verify if it's true or even related. BTW, in my fork. a year ago I updated zlib, so it may help too.

@digitalbox94
Copy link
Author

I adjusted the default value of TEMPLATE in my fork too.

That fno-define-target-os-macros is connected to zlib is my guess based on "cmake doesn't compile with clang 18 due to deprecated macros in zutil.c and gzguts.h" being the first result if I search for "fno-define-target-os-macros" on Google. And that thread promises that this problem is fixed in cmake 3.30. Can't verify if it's true or even related. BTW, in my fork. a year ago I updated zlib, so it may help too.

Ok for the TEMPLATE adjustment

Regarding the fno-define-target-os-macros yes it's linked to zlib compilation failing on MacOS.

I've upgraded cmake to version 3.30.2 but it still failing ( In file included from lib/zlib/gzguts.h:21:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk/usr/include/stdio.h:220:7: error: expected ')' )

I will check for all the other comments above and test them :)

@digitalbox94
Copy link
Author

  • QMAKE_CC = /usr/local/opt/llvm/bin/clang
    QMAKE_CXX = /usr/local/opt/llvm/bin/clang++
    INCLUDEPATH += /usr/local/opt/llvm/include

Ok please find below my findings :

  • QMAKE_CC / QMAKE_CXX / INCLUDEPATH: I've done an EXPORT of these variables outside of Nifskope.pro before launching qmake and make, but I don't know why it's not handled correctly. So clang/clang++ with latest version from homebrew is not taken into account (only clang/clang++ from Xcode). So compilation is still failing.
  • CONFIG += c++20 : on MacOS I don't know why but it's producing with qmake the option "-std=gnu++11" which is then make the compilation failing
  • ICON = nifskope.icns : yes this is only to have an icon of the application on MacOS

These options relative to Mac are handled with the command "macx" like below so that it doesn't disturb other platforms :
macx: {
QMAKE_CFLAGS = -fno-define-target-os-macros
}

I've put the PR here #87 but it might need some adjustments

@digitalbox94
Copy link
Author

Can I submit a PR for that ?

For what it is worth, I merged your changes to my fork here, although I cannot test them, because I only have Linux and Windows PCs. I did make a few modifications:

  • I set TEMPLATE = vcapp only for MSVC, and 'app' for everything else.
  • Removed the -fno-define-target-os-macros workaround, since I do not use zlib.
  • Moved the icon file to res/.
  • I also had problems with the same part of NifSkope_targets.pri in the past, so I already changed that code, but in a different way.

Ok I've tried to compile this fork and I have this result :

Everything is compiling fine except the file mesh.cpp for the below line

src/spells/mesh.cpp:1732:63: error: redefinition of parameter 'v'
 1732 |                                                 Triangle        t( quint16(v[p[0]]), quint16(v[p[1]]), quint16(v[p[2]]) );
      |                                                                                                                ^
src/spells/mesh.cpp:1732:27: note: previous declaration is here
 1732 |                                                 Triangle        t( quint16(v[p[0]]), quint16(v[p[1]]), quint16(v[p[2]]) );
      |                                                                            ^

I guess an adjustment is needed on the way v[] is used

@gavrant
Copy link

gavrant commented Aug 16, 2024

CONFIG += c++20 : on MacOS I don't know why but it's producing with qmake the option "-std=gnu++11" which is then make the compilation failing

I believe you still call the old qmake. It could be even your original 5.12.x (I guess, it's new enough to know about "c++2a"). What version of Qt is reported in your compiled NifSkope, Help menu, "About Qt"?

Also, just in case, if you have .qmake.stash file in your copy of NifSkope source code, try deleting it before running qmake next time. It appears to be a cash file for Qt, could be the culprit.

@gavrant
Copy link

gavrant commented Aug 16, 2024

QMAKE_CC / QMAKE_CXX / INCLUDEPATH: I've done an EXPORT of these variables outside of Nifskope.pro before launching qmake and make, but I don't know why it's not handled correctly. So clang/clang++ with latest version from homebrew is not taken into account (only clang/clang++ from Xcode). So compilation is still failing.

But if the changes from your PR are merged here, wouldn't they break NifSkope.pro for anyone on MacOS who tries to build it with XCode, g++ or any compiler other than LLVM clang?

@fo76utils
Copy link

Everything is compiling fine except the file mesh.cpp for the below line

I changed those lines, does this fix the error?

@digitalbox94
Copy link
Author

CONFIG += c++20 : on MacOS I don't know why but it's producing with qmake the option "-std=gnu++11" which is then make the compilation failing

I believe you still call the old qmake. It could be even your original 5.12.x (I guess, it's new enough to know about "c++2a"). What version of Qt is reported in your compiled NifSkope, Help menu, "About Qt"?

Also, just in case, if you have `.qmake.stash`` file in your copy of NifSkope source code, try deleting it before running qmake next time. It appears to be a cash file for Qt, could be the culprit.

I've checked the below :

qmake --version
QMake version 3.1
Using Qt version 5.15.13 in /usr/local/Cellar/qt@5/5.15.13_1/lib

About QT is giving : 5.15.13

There might be something relative to QT 5.12 but I don't see any remaining files (I have uninstalled it) :/
I will try to investigate this part (very strange).

"wouldn't they break NifSkope.pro for anyone on MacOS who tries to build it with XCode, g++ or any compiler other than LLVM clang?"

Yes you're correct, my assumption was that clang/clang++ is used to make things work on Mac

@digitalbox94
Copy link
Author

Everything is compiling fine except the file mesh.cpp for the below line

I changed those lines, does this fix the error?

Compilation is OK now :)
I can share a DMG for Mac if you want for your fork

@fo76utils
Copy link

Compilation is OK now :) I can share a DMG for Mac if you want for your fork

Thanks, I can add the DMG to the next release in a few days. It would be ideal though if you could write an Actions workflow for macOS, as I use those on both Linux and Windows for testing and for automatically generating release packages. For reference, main.yml runs on every 'git push', and release.yml on 'git push --tags', the latter creates the .7z packages I upload to releases.

@digitalbox94
Copy link
Author

Actions

I've never created such workflow, however it doesn't seem to be complex : I think for MacOS we could mimick the linux action workflow.
A question on that : once the file release.yml is updated for MacOS, how to test it ?

@digitalbox94
Copy link
Author

  • CONFIG += c++2a - I still believe that "c++20" should remain there because I've been compiling NifSkope with it for a year (on Windows though) without issues.

There's something strange, the QT docs are referring to c++20 for QT6 CONFIG and c++2a / c++2b for QT5 CONFIG : am I missing something here ?

QT5 : https://doc.qt.io/qt-5/qmake-variable-reference.html#config
QT6 : https://doc.qt.io/qt-6/qmake-variable-reference.html#config

@gavrant
Copy link

gavrant commented Aug 16, 2024

Yes, I'm starting to suspect that it's a Qt 5 thing and on some compilers/OSes qmake 5.x.x is fine with c++20 while on others it isn't. I wonder what Linux compiler fo76utils uses, if it is apparently good with c++20.

@fo76utils
Copy link

fo76utils commented Aug 16, 2024

A question on that : once the file release.yml is updated for MacOS, how to test it ?

You can test workflows in a fork of the repository, any .yml files that are placed under .github/workflows/ will be automatically run by GitHub whenever the conditions listed under "on:" are met. If the run fails, check the logs, those can be viewed for example here by clicking on "Build MSYS2" or "Build Linux", then expanding the details. The artifacts are .zip files that contain whatever was uploaded with actions/upload-artifact.

The workflow logs also show what packages were installed, for example the most recent Windows build used GCC 14.2.0 and Qt 5.15.14 (the last free release), while the Linux workflow was run on Ubuntu with Qt 5.15.3. For local builds on Linux, I use GCC 13.3.0 myself with Qt 5.15.14, and on Windows the same (latest) MSYS2 environment that is used by the workflows.

@fo76utils
Copy link

There's something strange, the QT docs are referring to c++20 for QT6 CONFIG and c++2a / c++2b for QT5 CONFIG : am I missing something here ?

This is correct, the 'CONFIG += c++20' does not actually seem to have any effect. The reason why GCC and MSVC still work is that for those compilers, NifSkope.pro adds the C++20 standard directly as a command line option around line 480 where compiler specific flags are handled.

@fo76utils
Copy link

I made some changes to NifSkope.pro so that Clang now uses similar command line options to GCC, and I also made the addition of x86 specific options conditional.

@fo76utils
Copy link

This is the actual error:

make: /usr/local/opt/llvm/bin/clang++: No such file or directory

It looks like an issue with the Clang paths.

@DigitalBox98
Copy link

DigitalBox98 commented Aug 17, 2024

Ok I"ve got a first version working :

Need to change the below clang for Nifskope.pro :

macx: {
QMAKE_CC = clang
QMAKE_CXX = clang++
}

And the main.yml for macOS :

  build_macos:
    runs-on: macos-latest
    name: Build MacOS

    steps:
    - name: 'Checkout'
      uses: actions/checkout@v4
      with:
        fetch-depth: 0
        submodules: recursive

    - name: 'Install required packages'
      uses: jurplel/install-qt-action@v2

    - name: 'Install clang'	  
      run: brew install llvm

    - name: 'Build with qmake'
      run: |
        qmake NifSkope.pro
        make -j 12

    - name: 'Upload Artifacts'
      uses: actions/upload-artifact@v4
      with:
        name: build-macos
        path: 'release'

Remaining is to use macdeployqt tool and before the copy of the xml file into the Nifskope.app//Contents/MacOS to have a full working DMG image

@DigitalBox98
Copy link

Latest action which is working fine for MacOS :

addition : copy of xml files + macdeployqt

There might be some adjustments to have a zip file containing the DMG image : release/NifSkope.dmg (not sure how it's created)

  build_macos:
    runs-on: macos-latest
    name: Build MacOS

    steps:
    - name: 'Checkout'
      uses: actions/checkout@v4
      with:
        fetch-depth: 0
        submodules: recursive

    - name: 'Install required packages'
      uses: jurplel/install-qt-action@v2

    - name: 'Install clang'	  
      run: brew install llvm

    - name: 'Build with qmake'
      run: |
        qmake NifSkope.pro
        make -j 12

    - name: 'Create DMG image'
      run: |
        cp release/*.xml release/NifSkope.app/Contents/MacOS/
        macdeployqt release/NifSkope.app -dmg

    - name: 'Upload Artifacts'
      uses: actions/upload-artifact@v4
      with:
        name: build-macos
        path: 'release'

@fo76utils
Copy link

Thanks, I added this to main.yml, and it does seem to successfully compile now. I made these changes:

  • Updated jurplel/install-qt-action to v4, as v2 is deprecated.
  • Added the 'noavx=1' option to qmake to fix compile errors.

I cannot test the .dmg in build-macos.zip, but assuming it does work, how should the files be packaged for a release version?

@camelotinsider
Copy link

I confirm the DMG image inside your zip artifacts is working fine on MacOS.

One possibility is to keep only the DMG file for the artifacts, and I have tested this change successfully :

    - name: 'Create DMG image'
      run: |
        cp release/*.xml release/NifSkope.app/Contents/MacOS/
        macdeployqt release/NifSkope.app -dmg
        mv release/NifSkope.dmg NifSkope.dmg
        rm -rf release/*
        mv NifSkope.dmg release/NifSkope.dmg

Once the workflow is completed, the artifacts can be downloaded and it will contain only the DMG image to install :)

nifskope

@fo76utils
Copy link

I changed the workflow file so it now only uploads the DMG as an artifact. I also added some previously missing files to the DMG, although some of these (like qt.conf and the .txt files) may not be needed.

gavrant added a commit to gavrant/nifskope that referenced this issue Aug 18, 2024
@digitalbox94
Copy link
Author

Ok I've checked the DMG image and I have some warnings relative to the shaders when launching the program :

shaders

@fo76utils
Copy link

fo76utils commented Aug 20, 2024

After searching for information on this issue, it looks like the GLSL version is limited to 120 on macOS, unless a core profile OpenGL 3.2+ context is used. NifSkope currently requests GL version 4.0, which would be enough for '#version 400' in the shaders, but it is compatibility profile (since a lot of code uses functions that were deprecated in OpenGL 3.x), and those are apparently not supported on macOS. So, the highest available versions are only OpenGL 2.1 and GLSL 120.

At least for the older games, it should be possible to modify the shaders to work with '#version 120'. The Starfield and possibly Fallout 76 shaders may need to be removed from the DMG for the time being.

@fo76utils
Copy link

I changed the workflow file to remove the Starfield shaders from the DMG, and all other shaders now only use GLSL 1.20. The noavx=1 workaround should also no longer be needed. The updated build can be found here, although I cannot test if the DMG works.

@digitalbox94
Copy link
Author

Thank you, it's working perfectly fine now without any warning messages :)

Maybe I can close this issue and corresponding PR #87 ?

@fo76utils
Copy link

The issue and pull request are still relevant to this repository (hexabits/nifskope), so they might not need to be closed yet.

By the way, I have now ported the Starfield shader as well to GLSL 1.20. Does this version work on macOS (or at least it compiles without errors if you do not have Starfield to test)?

@fo76utils
Copy link

fo76utils commented Aug 29, 2024

A preliminary Qt 6 build is now also available for testing here. Edit: I fixed the missing DLL in the Windows packages, and the Ubuntu workflow now finally also runs without errors.

@digitalbox94
Copy link
Author

Great job ! It's working fine on MacOS :

qt6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants