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

import macOS fixes from blaztinn/macos #33

Merged
merged 7 commits into from
Feb 11, 2023
Merged

import macOS fixes from blaztinn/macos #33

merged 7 commits into from
Feb 11, 2023

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Feb 10, 2023

Import macOS fixes from blaztinn/macos, see:

The crnlib was already buildable on macOS when integrated in another software (like the Dæmon engine or the Netradiant editor), but not the crunch command line tool.

The branch also features some other minor fixes around that are probably not bad.

The macOS crunch tool builds and runs, tested on amd64 macOS Mojave.


Part of original message:

This PR still builds on Linux and Windows.

This PR doesn't build on macOS yet because cd8353d introduced this:

In file included from crunch/crnlib/crn_threading.h:7:
crunch/crnlib/crn_threading_pthreads.h:177:40: warning: defaulted function
      definitions are a C++11 extension [-Wc++11-extensions]
    virtual ~executable_task( void ) = default;
                                       ^
crunch/crnlib/crn_threading_pthreads.h:253:11: error: exception specification
      of overriding function is more lax than base version
  virtual ~object_task( void ) = default;
          ^
crunch/crnlib/crn_threading_pthreads.h:177:13: note: overridden virtual
      function is here
    virtual ~executable_task( void ) = default;
            ^
crunch/crnlib/crn_threading_pthreads.h:253:34: warning: defaulted function
      definitions are a C++11 extension [-Wc++11-extensions]
  virtual ~object_task( void ) = default;
                                 ^
make[2]: *** [crnlib/CMakeFiles/crn-obj.dir/crn_dds_comp.cpp.o] Error 1
make[2]: *** [crnlib/CMakeFiles/crn-obj.dir/crn_image_utils.cpp.o] Error 1
make[2]: *** [crnlib/CMakeFiles/crn-obj.dir/crn_dxt_image.cpp.o] Error 1

If I temporarily revert cd8353d the build continues but then the linkage fails:

[ 92%] Built target crn-obj
[ 94%] Linking CXX shared library libcrn.dylib
[ 94%] Linking CXX static library libcrn.a
duplicate symbol __ZN6crnlib5utils10write_le64EPvy in:
    CMakeFiles/crn-obj.dir/crn_arealist.cpp.o
    CMakeFiles/crn-obj.dir/crn_assert.cpp.o
[…]
ld: 994 duplicate symbols for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

So,

  1. We need to fix the exception specification of overriding function is more lax than base version error. @bmorel can you look at it? You're the one having introduced the commit triggering it so maybe you know what those errors are about… 🙂️

  2. We need to fix the linkage error.

Instead of `pthread_spinlock_t` use `os_unfair_lock_t` because macos
is missing the pthread spinlock.

`sem_init` is deprecated on macos and always returns `-1`. Use
`sem_open` together with `sem_unlink` to open an exclusive sempahore in
global namespace.

Use `sem_wait` because `sem_timedWait` is missing on macos.
The commit `4bb735f7966eedf414aa8e24533967cffd5bbcdb` only fixed the
threading limit on the decoder part. Use the limit also in the encoder.
Avoids errors on macOS like:

In file included from crunch/crnlib/crn_threading.h:7:
crunch/crnlib/crn_threading_pthreads.h:177:40: warning: defaulted function
      definitions are a C++11 extension [-Wc++11-extensions]
    virtual ~executable_task( void ) = default;
                                       ^
crunch/crnlib/crn_threading_pthreads.h:253:11: error: exception specification
      of overriding function is more lax than base version
  virtual ~object_task( void ) = default;
          ^
crunch/crnlib/crn_threading_pthreads.h:177:13: note: overridden virtual
      function is here
    virtual ~executable_task( void ) = default;
            ^
crunch/crnlib/crn_threading_pthreads.h:253:34: warning: defaulted function
      definitions are a C++11 extension [-Wc++11-extensions]
  virtual ~object_task( void ) = default;
                                 ^
@illwieckz illwieckz changed the title Import macos fixes from blaztinn/macos import macos fixes from blaztinn/macos Feb 10, 2023
…r will override anyway

Avoids errors on macOS like:

duplicate symbol __ZN6crnlib5utils10write_le64EPvy in:
    crn_arealist.o
    crn_assert.o

Setting static instead of extern would also workaround the error but it's wrong
and would raise warnings on other systems like FreeBSD:

In file included from crunch/crnlib/crn_core.h:174:
crunch/crnlib/crn_utils.h:235:8: warning: 'gnu_inline' attribute without 'extern' in C++ treated as externally available, this changed in Clang 10 [-Wgnu-inline-cpp-without-extern]
static CRNLIB_FORCE_INLINE void write_be64(void* p, uint64 x) {
       ^
crunch/crnlib/crn_core.h:85:70: note: expanded from macro 'CRNLIB_FORCE_INLINE'
 #define CRNLIB_FORCE_INLINE inline __attribute__((__always_inline__, __gnu_inline__))
                                                                      ^

Setting extern fixes the duplicate symbols error on macOS without introducing
warnings on FreeBSD.
@illwieckz
Copy link
Member Author

illwieckz commented Feb 10, 2023

Ok, it now builds.

There is one bug, though. The crunch tool works when the image is stored on a disk, not on NFS, maybe it's not a macOS bug but a generic crunch bug (I would have to test NFS on other platforms):

$ pwd
/Users/illwieckz/Mapping/Unvanquished/UnvanquishedAssets/src/tex-pk01_src.dpkdir/textures/shared_pk01_src

$ crunch -file door01_a.png -out door01_a.crn
Warning: No files found: /Users/illwieckz/Mapping/Unvanquished/UnvanquishedAssets/src/tex-pk01_src.dpkdir/textures/shared_pk01_src/door01_a.png
Error: No files found to process!

$ cd

$ pwd
/Users/illwieckz

$ cp -a /Users/illwieckz/Mapping/Unvanquished/UnvanquishedAssets/src/tex-pk01_src.dpkdir/textures/shared_pk01_src/door01_a.png

$ crunch -file door01_a.png -out door01_a.crn
Reading source texture: "/Users/illwieckz/door01_a.png"
Texture successfully loaded in 0.002s
Source texture: 256x512, Levels: 1, Faces: 1, Format: R8G8B8
Apparent type: 2D map, Flags: R G B Non-Flipped 
Generating mipmaps using filter "kaiser"
Generated 9 mipmap levels in 0.042s
Writing DXT1 texture to file: "door01_a.crn"

@illwieckz illwieckz changed the title import macos fixes from blaztinn/macos import macOS fixes from blaztinn/macos Feb 10, 2023
@illwieckz
Copy link
Member Author

illwieckz commented Feb 10, 2023

I also imported that other fix by @blaztinn from:

It's a complement for 4bb735f

@illwieckz
Copy link
Member Author

illwieckz commented Feb 11, 2023

Maybe the bug about reading file on NFS on macOS is not a macOS specific bug but a generic crunch bug about the way it does file access and specificity of network file system, I would have to test NFS on other platforms to know more. I don't see that issue as a blocking issue for this merge, as the tool can actually be built and converts properly images to and from crn on macOS.

It is known crunch for Windows is able to read and write files on Windows native CIFS network share, but this is not NFS.

After the macOS patches are applied the tool still builds and run on Linux (tested with GCC) and Windows (tested with MSYS2/Mingw).

I consider this PR ready.

@illwieckz
Copy link
Member Author

illwieckz commented Feb 11, 2023

I tested on amd64 macOS Mojave.

A test on arm64 Apple with newer macOS would also be nice.

@illwieckz
Copy link
Member Author

illwieckz commented Feb 11, 2023

As an alternative way to the extern CRNLIB_FORCE_INLINE stuff, we may also be able to do this (from this other branch implementing macOS support):

#ifdef __APPLE__
#define CRNLIB_FORCE_INLINE inline __attribute__((__always_inline__))
#else
#define CRNLIB_FORCE_INLINE inline __attribute__((__always_inline__, __gnu_inline__))
#endif

I haven't tested though.

But before I replaced existing static CRNLIB_FORCE_INLINE and CRNLIB_FORCE_INLINE usages with extern CRNLIB_FORCE_INLINE, existing static CRNLIB_FORCE_INLINE was already raising some warning on FreeBSD because the options used by CRNLIB_FORCE_INLINE are already enforced to be extern in all cases. So while I don't understand what's happening, the solution implemented in this PR (static CRNLIB_FORCE_INLINE) both fixes the build on macOS and silence warnings on FreeBSD.

Copy link

@DolceTriade DolceTriade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't test but seems ok

@illwieckz
Copy link
Member Author

illwieckz commented Feb 11, 2023

I noticed this implementation, like this other one or this other one is implementing the second fix discussed in #14 in addition to the first one we already do.

I guess it's harmless, and this other macOS patch for macOS is know to already do both fixes. That people especially applied the second fix above our own fix, so maybe that was intentional.

The two macOS ports we know both do that second fix, above or not above our first fix. So doing that second fix as well reduces diff noise between trees.

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

Successfully merging this pull request may close these issues.

3 participants