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.

Sign up for GitHub

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

Jump to bottom

unix only headers #3

Closed
minimalisticMe opened this issue May 4, 2016 · 32 comments
Closed

unix only headers #3

minimalisticMe opened this issue May 4, 2016 · 32 comments

Comments

@minimalisticMe
Copy link

minimalisticMe commented May 4, 2016 •
edited
Loading

Hey,
your project uses two unix only includes:

  • inttypes.h
  • unistd.h

For the unistd.h in lib/xlsxio_write.c there is an easy solution, as there is an equivalent header. Replace unistd.h (line 8) with:

#ifdef _WIN32
    #include <io.h>
#else
    #include <unistd.h>
#endif

For inttypes.h in lib/xlsxio_write.c and lib/xlsxio_read.c I'm still searching for a solution, as it is not officially in windows, there is a port here, which I'm not a big fan of. It would be better to find the used defines and define them with an #ifdef like the code snipped above.
e.g. (probably did not get all defines)

#ifdef _WIN32
    #define PRIi64 "I64i"
#else
    #include <inttypes.h>
#endif
@brechtsanders
Copy link
Owner

Hi,
The project builds perfectly with MinGW (#ifdef MINGW32).
Maybe this is something Visual C doesn't have.
Where does Visual C define uint64_t (instead of inttypes.h)?
Regards
Brecht

On 4/05/2016 15:58, minimalisticMe wrote:

Hey,
your project uses two unix only includes:

  • inttypes.h
  • unistd.h

For the /unistd.h/ in /lib/xlsxio_write.c/ there is an easy solution,
as there is an equivalent header. Replace /unistd.h/ (line 8) with:

|#ifdef _WIN32 #include <io.h> #else #include <unistd.h> #endif |

For /inttypes.h/ I'm still searching for a solution, as it is not
officially in windows, there is a port here
https://code.google.com/archive/p/msinttypes/downloads, which I'm
not a big fan of. It would be better to find the used defines and
define them with an /#ifdef/ like the code snipped above.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#3

@minimalisticMe
Copy link
Author

I do know, that the absence of unistd.h is Visual Studio specific. One could check for Visual Studio (MSVC) via #if _MSC_VER >= 1600 instead of #ifdef _WIN32 (where version 1600 is Visual Studio 2010)

@minimalisticMe
Copy link
Author

As VS C does have neither inttypes.h nor stdint.h I usually define it myself, when I need it. In shared projects, I usually do the following:

#ifdef _MSC_VER
    typedef __int32 int32_t;
    typedef unsigned __int32 uint32_t;
    typedef __int64 int64_t;
    typedef unsigned __int64 uint64_t;
#else
    #include <stdint.h>
#endif

@brechtsanders
Copy link
Owner

It looks like unistd.h isn't needed on Windows and io.h doesn't work on
Linux and OS X.
So I changed it to:
#ifndef _WIN32
#include <unistd.h>
#endif

On 4/05/2016 15:58, minimalisticMe wrote:

Hey,
your project uses two unix only includes:

  • inttypes.h
  • unistd.h

For the /unistd.h/ in /lib/xlsxio_write.c/ there is an easy solution,
as there is an equivalent header. Replace /unistd.h/ (line 8) with:

|#ifdef _WIN32 #include <io.h> #else #include <unistd.h> #endif |

For /inttypes.h/ I'm still searching for a solution, as it is not
officially in windows, there is a port here
https://code.google.com/archive/p/msinttypes/downloads, which I'm
not a big fan of. It would be better to find the used defines and
define them with an /#ifdef/ like the code snipped above.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#3

@brechtsanders
Copy link
Owner

According to https://msdn.microsoft.com/en-us/library/323b6b3k.aspx
there is stdint.h in Visual Studio 2015.

On 4/05/2016 16:22, minimalisticMe wrote:

I do know, that the absence of /unistd.h/ is Visual Studio specific.
One could check for Visual Studio (MSVC) via |#if _MSC_VER >= 1600|
instead of |#ifdef _WIN32| (where version 1600 is Visual Studio 2010)

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#3 (comment)

@brechtsanders
Copy link
Owner

Here's how I have done it:

#if defined(_MSC_VER) && _MSC_VER < 1600
typedef signed __int64 int64_t;
#else
#include <stdint.h>
#endif

Can you try building 0.2.1?

On 4/05/2016 16:26, minimalisticMe wrote:

As VS C does have neither /inttypes.h/ nor /stdint.h/ I usually define
it myself, when I need it. In shared projects, I usually do the following:

|#ifdef _MSC_VER typedef __int32 int32_t; typedef unsigned __int32
uint32_t; typedef __int64 int64_t; typedef unsigned __int64 uint64_t;
#else #include <stdint.h> #endif |

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#3 (comment)

@minimalisticMe
Copy link
Author

minimalisticMe commented May 6, 2016 •
edited
Loading

I will try building v0.2.1 later and give you some feedback.
Unfortunately your snipped above is not completely correct, as Visual Studio 2015 has _MSC_VER set to 1900, so it should read

#if defined(_MSC_VER) && _MSC_VER < 1900 
  typedef signed __int64 int64_t; 
#else 
  #include <stdint.h>
#endif

@brechtsanders
Copy link
Owner

That doesn't seem correct as Microsoft says here:
https://msdn.microsoft.com/en-us/library/323b6b3k.aspx
that stdint.h is included in Visual Studio 2015.
Are you sure you don't have it (maybe in a non-standard include folder)?

On 6/05/2016 10:13, minimalisticMe wrote:

I will try building v0.2.1 later and give you some feedback.
Unfortunately your snipped above is not completely correct, as Visual
Studio 2015 has |_MSC_VER| set to |1900|, so it should read

|#if defined(_MSC_VER) && _MSC_VER < 1600 typedef signed __int64
int64_t; #else #include <stdint.h> #endif |

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#3 (comment)

@minimalisticMe
Copy link
Author

That is right, VS2015 does support stdint.h.
Unfortunately I'm using some old libraries, that make me use VS2010 until the upgrade to modern c++ standard in those libs is complete :(

@minimalisticMe
Copy link
Author

Update: I'm now building xlsxio via VS2015 and there is support for inttypes.h (which is needed for PRIi64 in xlsxio_read.c and xlsxio_write.c, so the _#ifndef WIN32 in line 4 can be removed and #include <inttypes.h> in line 8 of xlsxio_write.c can be moved outside of _#ifndef WIN32

@brechtsanders
Copy link
Owner

Fixed in 0.2.2

@jmporter34
Copy link

Hi,
I'm building on Windows 7 with Visual Studio Community 2017 and getting
Cannot open include file: 'unistd.h' in xlsio_read.c.

I see that xlsxio_write.c now contains
#ifndef _WIN32
#include <unistd.h>
#endif
But xlsxio_read.c does not contain the conditional around it. Could you please add it ?
Thanks

BTW I wasn't sure whether to comment on this closed issue or open a new one. I figured I would try this first.

@brechtsanders
Copy link
Owner

Hi,
I was using MinGW on Windows, so Visual Studio was not tested.
If I don't use #include <stdio.h> then I get errors about missing read/close/lseek.
Does Visual Studio need another include instead for these functions?
Regards
Brecht

@jmporter34
Copy link

Hi,
For a workaround, I tried creating an empty file named "unistd.h" in the include path, then xlsxio_read.c compiled with no issues other than some deprecated function name warnings.

The build did fail somewhere later. A libzip file named zip_crypto.h generated an error "no crypto backend found". I'm still trying to figure that one out, but obviously it's unrelated to the "unistd.h" issue.

Thanks for the quick response.

@jmporter34
Copy link

By the way, VS 2017 Community is free for individuals, so if you're interested in testing your code against it there's no cost involved (other than your time and significant space on your hard drive).

@brechtsanders
Copy link
Owner

I'm not that interested in having a Microsoft compiler when I can have gcc or llvm compiler that works an a lot more platforms.

libzip is deprecated, from now on XLSX I/O will focus on minizip instead.

Can't you just use my precompiled DLL and link with that using Visual Studio?

@jmporter34
Copy link

jmporter34 commented Mar 28, 2018 via email

@brechtsanders
Copy link
Owner

Deprecated maybe was a big word, but the fact is that LibreOffice just can't open the files generated with libzip, so I kind of had to switch to something else and minizip seems to do just fine, even though its code does seem a bit primitive. Also, it did make into it's way into Linux distributions, so it seems like it's also commonly used.
Maybe libarchive would have been an alternative, but I wanted to avoid the bloat since I just need zip functionality...
Let me know how it goes using the DLL from the binary downloads.

@jmporter34
Copy link

jmporter34 commented Mar 28, 2018 via email

@brechtsanders
Copy link
Owner

Are you sure you are using the latest source? It doesn't seem like it.
Make sure USE_MINIZIP is defined when building.
For minizip it needs the include path where minizip is a subfolder.

@jmporter34
Copy link

jmporter34 commented Mar 28, 2018 via email

@brechtsanders
Copy link
Owner

What I meant was: are you using XLSX I/O sources version 0.2.16 ?
Latest minizip I could find was 1.2.8 from this link: http://www.gaia-gis.it/gaia-sins/dataseltzer-sources/
On my Debian Linux system minzip is version 1.1-8 which works just fine too.

Anyway, you shouldn't be using zip.h but minizip/zip.h and minizip/unzip.h from which zipFile and unzFile are used instead of zip_file_t, unless you are using the wrong sources or haven't compiled with /DUSE_MINIZIP

@jmporter34
Copy link

jmporter34 commented Mar 28, 2018 via email

@brechtsanders
Copy link
Owner

minizip 1.1 should be just fine.
zip.h is for libzip, minizip/zip.h is for minizip, there should be no mixup as logn as you don't include the minizip folder itzelf.
zip_file_t is from libzip and is not defined in minizip, which uses zipFile/unzFile instead. The USE_MINIZIP define is used to make sure the right one is used.
I release Windows 32/64 bit binaries with every release, see: https://github.com/brechtsanders/xlsxio/releases

@jmporter34
Copy link

You said "zip_file_t is from libzip and is not defined in minizip, which uses zipFile/unzFile instead. The USE_MINIZIP define is used to make sure the right one is used."

But 'xlsxio_read.c' includes 'xlsxio_read_sharedstrings.h' with no '#ifdef USE_MINIZIP' around the include. And 'xlsxio_read_sharedstrings.h' references 'zip_file_t' with no '#ifdef USE_MINIZIP' around the reference. So if you're including the minizip 'zip.h' rather than the libzip 'zip.h', it seems like you're guaranteed to get a compile error no matter what compiler you're using.

I didn't realize that I had to click on 'Releases' on the xlsxio page to get to your DLL - still learning my way around Github.
Thanks again for all the help.

@brechtsanders
Copy link
Owner

Sorry, but I'm not seeing what you're saying in the code.
To be sure we're looking at the same thing, can you look at https://github.com/brechtsanders/xlsxio/blob/master/lib/xlsxio_read_sharedstrings.c and tell me where exactly it goes wrong?

@jmporter34
Copy link

I was actually talking about getting errors compiling 'xlsxio_read.c' rather than 'xlsxio_read_sharedstrings.c', but the result is the same. The first two lines of both files are identical:
#include "xlsxio_private.h"
#include "xlsxio_read_sharedstrings.h"

Line 5 of 'xlsxio_read_sharedstrings.h' includes <zip.h>. Since we're building with minizip rather than libzip, my include paths include the minizip folder but not the libzip folder (and I confirmed that I'm including the correct version of <zip.h>, i.e. the minizip version, as I mentioned earlier).

Line 28 of 'xlsxio_read_sharedstrings.h' references 'zip_file_t', which is only defined in the libzip version of zip.h, i.e. not the version that we're including. The reference to 'zip_file_t' is not surrounded by #ifndef USE_MINIZIP. So it seems like line 28 will always generate a compile error when you build with minizip, no matter what compiler you're using.

Note: in my earlier attempt to describe the issue, I had said that 'xlsxio_read_sharedstrings.h' references 'zip_file_t' without #ifdef USE_MINIZIP around the reference. It should have said without #ifndef USE_MINIZIP. I hope this clarifies what I'm trying to say.

@brechtsanders
Copy link
Owner

Got it.
This was indeed wrong, but I hadn't noticed because I had both libzip and minizip in my include path on all my test platforms (Windows, Linux, macOS).
I have now corrected the sources in subversion. These fixes will be part of the next release.

@jmporter34
Copy link

jmporter34 commented Mar 29, 2018 via email

@brechtsanders
Copy link
Owner

That #include <unistd.h> was needed for low level file I/O, specifically functions read/close/lseek. If you know where these are in MSVC I can look into that.

By all means, if you have suggestions on how to make it compatible and this can be done without affecting other OSes I would be happy to make the necessary changes.

@fengcai
Copy link

fengcai commented Dec 14, 2018

Great, thank you ! There was that one other thing, where xlsxio_read.c includes <unistd.h> without #ifndef _WIN32 around it (you had added the conditional in xlsxio_write.c but not in xlsxio_read.c). I have a workaround for that. More generally, now that I'm past the minizip-related issue, I'm finding some compatibility-related issues. e.g. xlsxio_read.c refers to ssize_t, which apparently is GNU-specific and isn't defined in Visual Studio. (there is SSIZE_T, but I haven't dug in yet to figure out if they mean the same thing). So, if you're interested in adding VS compatibility, I can update you later with all the things that I've had to muck with to make xlsxio work with VS. And you can decide if you want to incorporate any of them. If you don't care about VS, I'll just hack up my local copy of xlsxio as needed. And/or try your precompiled DLL. And/or give MinGW a try. Either way, thanks for all the work you've put into xlsxio and for making it available on Github.
…
On Thu, Mar 29, 2018 at 7:13 AM, Brecht Sanders @.***> wrote: Got it. This was indeed wrong, but I hadn't noticed because I had both libzip and minizip in my include path on all my test platforms (Windows, Linux, macOS). I have now corrected the sources in subversion. These fixes will be part of the next release. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/Ai0l9664xPcgvDPaPPIF0eUvps3lh86Hks5tjMHugaJpZM4IXK5w .

hi jmporter34,

can you share your VC project, I failed to compile with VC, and I tryied 2012/5?

thanks.

@brechtsanders
Copy link
Owner

The only reason ssize_t is used in xlsxio_read.c is because that's the return type of read() which requires <unistd.h>. In MSVC that can be replaced with _read() from <io.h>.
I just committed this change. Can you try to build the current master sources?

@stupid0319 stupid0319 mentioned this issue Dec 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fengcai @minimalisticMe @brechtsanders @jmporter34