recp / AssetKit

🎨 Modern 2D/3D - Importer • Exporter • Util - Library, also known as (AssetIO)
Apache License 2.0
185 stars 18 forks source link

use __declspec for visibility on mingw #29

Closed recp closed 2 years ago

recp commented 2 years ago

Reference answer: https://stackoverflow.com/a/9939664/2676533

Related issue: https://github.com/recp/AssetKit/issues/27

eLeCtrOssSnake commented 2 years ago

Mingw complains about visibility attribute only in src/ds/default/default.c:24

deps\ds\src\default\default.c:28:1: error: visibility attribute not supported in this configuration; ignored [-Werror=attributes]

No _MSC_VER does not mean linux thing missed at src/utils.c:72 where st_blksize is st_size on mingw.

src\utils.c:72:23: error: 'struct stat' has no member named 'st_blksize'; did you mean 'st_size'?
   72 |   blksize = infile_st.st_blksize;
      |                       ^~~~~~~~~~
      |                       st_size

And a lot of _MSC_VER defines in src/mmap.c and incorrect mmap functions/defs.

src\mem\mmap.c:20:12: fatal error: sys/mman.h: No such file or directory
   20 | #  include <sys/mman.h>
      |            ^~~~~~~~~~~~

If I hand fix these defines everything builds. (These are results of static compilation)

eLeCtrOssSnake commented 2 years ago

Shared build works much better, but it fails at final linking stage:

objects.a(utils.c.obj):utils.c:(.text+0x206): undefined reference to `strptime'
collect2.exe: error: ld returned 1 exit status
CMakeFiles\assetkit.dir\build.make:2768: recipe for target 'libassetkit.dll' failed
mingw32-make[2]: *** [libassetkit.dll] Error 1
CMakeFiles\Makefile2:662: recipe for target 'CMakeFiles/assetkit.dir/all' failed
mingw32-make[1]: *** [CMakeFiles/assetkit.dir/all] Error 2
Makefile:154: recipe for target 'all' failed
mingw32-make: *** [all] Error 2

By some reason it did not build and/or link strptime

recp commented 2 years ago

Mingw complains about visibility attribute only in src/ds/default/default.c:24

I added this chage to libds too:

#if defined(_MSC_VER) || defined(__MINGW32__) || defined(__MINGW64__)
#  ifdef DS_STATIC
#    define DS_EXPORT
#  elif defined(DS_EXPORTS)
#    define DS_EXPORT __declspec(dllexport)
#  else
#    define DS_EXPORT __declspec(dllimport)
#  endif
#  define DS_HIDE
#else
#  define DS_EXPORT  __attribute__((visibility("default")))
#  define DS_HIDE    __attribute__((visibility("hidden")))
#endif

it now must be work.

recp commented 2 years ago

And a lot of _MSC_VER defines in src/mmap.c and incorrect mmap functions/defs.

There are lot of _MSC_VER usages, maybe we can define it as:

#if defined(_MSC_VER) || defined(__MINGW32__) || defined(__MINGW64__)
#  define AK_COMPILER_MSC
#endif

/* ad use AK_COMPILER_MSC instead of  _MSC_VER in all places */

#if AK_COMPILER_MSC
...
#endif  

but I'm not sure if it is the right path to go

eLeCtrOssSnake commented 2 years ago

Also to note it does not include strptime in static build so you also have to include the strptime.c file into the project.

eLeCtrOssSnake commented 2 years ago

But I have checked CI and it seemed like it broke msvc or something?

eLeCtrOssSnake commented 2 years ago

Let me pull the changes and see what kind of results I get.

eLeCtrOssSnake commented 2 years ago

There's still that utils.c error complaining about struct stat

\src\utils.c: In function '[ 20%] ak_readfileBuilding C object CMakeFiles/assetkit.dir/src/bbox/mesh_prim.c.obj                                                                                                         ':                                                                                                                      D:\!Development\AssetKit\src\utils.c:72:23: error: 'struct stat' has no member named 'st_blksize'; did you mean 'st_siz '?                                                                                                                         72 |   blksize = infile_st.st_blksize;                                                                                     |                       ^~~~~~~~~~                                                                                      |                       [ 20%] st_size
eLeCtrOssSnake commented 2 years ago

Pretty much AK_COMPILER_MSC would work apart from the src/utils.c somehow having a different struct member name. Only the util.c and mmap.c have old defs.

eLeCtrOssSnake commented 2 years ago

So then at src/utils.c:71 it should be:

#ifndef AK_COMPILER_MSC
  #ifndef _MSC_VER
    blksize = infile_st.st_size;
  #else
    blksize = infine_st.st_blksize;
  #endif
#else
  blksize = 512;
#endif

And at src/mem/mmap.c:19 it should be:

#ifndef AK_COMPILER_MSC
#  include <sys/mman.h>
#else
#  define WIN32_LEAN_AND_MEAN
#  include <windows.h>
#  include <io.h>
#endif
recp commented 2 years ago

What about this commit: https://github.com/recp/AssetKit/pull/29/commits/2996c5df74638894b563d5e6531c40b41fa3385a ?

recp commented 2 years ago

By some reason it did not build and/or link strptime

Let me check

recp commented 2 years ago

We must include src/win32/ folder into build if it is mingw

recp commented 2 years ago

@eLeCtrOssSnake can you pull and try again please?

eLeCtrOssSnake commented 2 years ago

Trying out.

eLeCtrOssSnake commented 2 years ago

blksize = infile_st.st_blksize in mingw must be blksize = infile_st.st_size or 512. No such property st_blksize

recp commented 2 years ago

It passed on my Windows with mingw, no compiling errors?

recp commented 2 years ago

Did you pull the PR with newest commits?

eLeCtrOssSnake commented 2 years ago

Woopsie, I messed up the stash. Yes, it compiles properly now. Seems like the mingw build is good. Checked msvc build just in case, also built fine.

recp commented 2 years ago

@eLeCtrOssSnake many thanks for help. I'll try to update morph targets and material system asap... Feel free to bring any other issues by creating issue/pr...

eLeCtrOssSnake commented 2 years ago

@eLeCtrOssSnake many thanks for help. I'll try to update morph targets and material system asap... Feel free to bring any other issues by creating issue/pr...

Yes, I will be watching AssetKit closely, because I want to migrate my engine to it and stop using assimp. I'd want to chat with you in private. I wrote an email but you did not respond @recp. Can I get in touch with you outside github?

recp commented 2 years ago

I want to migrate my engine

great to hear that!

With this repo (https://github.com/recp/assetkit-gl) I integrated AssetKit to my render engine, you may check it how to use AssetKit (until docs are finished) or you can use it another way maybe... But it may be updated in the future and I'm considering to integrate AssetKit, my render engine[s], physics engine... in a engine called Universal Engine which is private for now but it may be public in the future, it that case assetkit-gl may become obsolete or stay as another library maybe...

I wrote an email but you did not respond @recp. Can I get in touch with you outside github?

I didn't check emails last days I guess, I'll do that later... but it is better to continue onn Github which may help to others' issues...

eLeCtrOssSnake commented 2 years ago

I obviously won't hide stuff that would benefit the public. It's obviously something I wouldn't like to disclose or leave here.

recp commented 2 years ago

I'll check mails later, sorry for the delay then