MSVC/Windows support #214

zebrys · 2020-09-07T16:22:27Z

This PR enable Microsoft Visual Studio (>=2017) to build the project and use it on Windows.

# Conflicts: # .gitignore

evanmiller · 2020-09-07T17:07:09Z

Hi, thank you for the contribution. It appears the travis and appveyor builds are failing - examining the output, "exit code 139" indicates a segfault. So there is likely a memory or double-free error somewhere.

Look at the code, I think it's better to have a single logic path (where possible) for both Windows and non-Windows. So I'm OK using malloc() instead of variable-sized stack allocations everywhere in order to meet MSVC's requirements. I recommend removing the non-_MSC_VER paths that aren't necessary.

Going forward, we will need to ensure that future changes do not break the MSVC build. So I also recommend changing the Appveyor configuration to build with an MSVC toolset. I'm not overly versed in Appveyor, but I can try to provide assistance there if you need it.

…/realloc for all archs

evanmiller

This is looking better, thank you for making the changes. I have suggested a few more. The only mandatory one is fixing READSTAT_VERSION.

Before approval, we will need to set up a build with MSVC on Appveyor (as mentioned previously).

evanmiller · 2020-09-08T11:05:55Z

src/bin/readstat.c

+#if !defined _MSC_VER
+#   include <sys/time.h>
+#else
+#   define READSTAT_VERSION "0.2.0"


The version should not be hard-coded. Currently we maintain version information in configure.ac.

evanmiller · 2020-09-08T11:11:28Z

src/bin/readstat.c

            unlink(output_filename);
+#else
+            _unlink(output_filename);
+#endif


For platform differences like this, I would prefer to move the logic somewhere else so that it can be reused. For a simple case like this probably adding to the top of the file

#if defined _MSC_VER #define unlink _unlink #endif

Would be fine.

evanmiller · 2020-09-08T11:15:01Z

src/bin/readstat.c

+        struct tm ltm;
+        localtime_s(&ltm, &timestamp);
+        strftime(buffer, sizeof(buffer), "%d %b %Y %H:%M", &ltm);
+#endif


For a more complicated platform difference like this, a separate function is probably warranted, e.g.

size_t readstat_strftime(char *s, size_t maxsize, const char *format, time_t timsetamp) { #if !defined _MSC_VER ... #else ... #endif }

That way we keep _MSC_VER out of the main application functions.

evanmiller · 2020-09-08T11:15:53Z

src/bin/write/mod_csv.c

-#include <unistd.h>
+#if !defined(_MSC_VER)
+#   include <unistd.h>
+#endif


I am guessing that most inclusions of unistd.h are no longer needed even on Unix, but I can clean them up later.

evanmiller · 2020-09-08T11:18:00Z

src/sas/readstat_sas7bcat_read.c

@@ -133,6 +134,9 @@ static readstat_error_t sas7bcat_parse_value_labels(const char *value_start, siz
    }

 cleanup:
+    if(label)
+        free(label);


It's not important, but for future reference: free will accept a NULL pointer, so it's not necessary to check it with if(label).

evanmiller · 2020-09-08T11:18:33Z

src/sas/readstat_sas7bdat_write.c

-        char header[hinfo->page_header_size];
-        memset(header, 0, sizeof(header));
+        char *header = malloc(hinfo->page_header_size);
+        memset(header, 0, hinfo->page_header_size);


malloc/memset should use calloc(1, hinfo->page_header_size) instead

evanmiller · 2020-09-08T11:27:48Z

CC @jonathon-love

zebrys · 2020-09-08T13:48:35Z

Thank you for your feedback.
Regarding READSTAT_VERSION, I think the only way to do the same on MSVC would be to put it in the MSVC configuration file. Thus, in the end, there will be a READSTAT_VERSION define in the autotools and one in the MSVC file. Is it OK for you ?

evanmiller · 2020-09-08T16:04:10Z

@zebrys Thanks for making the requested changes. Go ahead and add READSTAT_VERSION to the MSVC file and I'll see if it looks manageable from my end. (Current version is 1.1.4.)

jonathon-love · 2020-09-08T22:20:05Z

ah! this looks great! i maintain a patch for MSVC support over here:

https://github.com/jonathon-love/ReadStat

but it looks like you've got it all covered anyway. let me see if i can simply use this as a drop-in replacement.

jonathon-love · 2020-09-08T22:58:02Z

hi,

i'm having some failures, which i assume is to do with me using an earlier version of MSVC.

https://ci.appveyor.com/project/jonathon-love/jamovi-readstat

i'm using 14 (because we want python compatibility ... @ofajardo too). it looks like the earlier version doesn't support the restrict keyword. this is how i work around it:

jonathon-love@c77cbff#diff-34dc7b78affe90ca491a0d0492d933ee

how would people feel about me making this compatible with MSVC 14?

jonathon

evanmiller · 2020-09-08T23:08:56Z

All versions are fine with me, as long as we get CI / Appveyor coverage in the end. The config file from the Boost::Math project may provide some guidance, as they cover both MSVC and GCC:

https://github.com/boostorg/math/blob/develop/appveyor.yml

zebrys · 2020-09-09T15:14:08Z

The VS issue was due to a problem during a merge on our side. This last update is now tested with VS2017 and works well. It should also theoretically work with VS2019 and probably VS2014/VS2015 but the best would be to check. The restrict issue has been fixed as well (as a missing VLA use). @jonathon-love can you check the last update on VS2014?

zebrys · 2020-09-09T17:19:14Z

@evanmiller We never used Appveyor so far. We will try to do that.

evanmiller · 2020-09-09T17:51:31Z

@zebrys Okay, that would be great!

zebrys · 2020-09-09T17:59:51Z

The iconv dependency is a problem for the CI on Windows: it does not seems to be already available on the image and is required for the MSVC project (we just need a directory with the header and the compiled lib files). What is the standard way to get dependencies like this in such CI ?

evanmiller · 2020-09-09T18:13:04Z

Will this solve your problem?

https://github.com/fastlane/fastlane/pull/13191/files

We could also explore a native Win32 solution since almost all transcoding is routed through readstat_convert.

jonathon-love · 2020-09-09T23:09:19Z

wrt iconv, you could consider my approach. i mount these projects as submodules:

https://github.com/kiyolee/libiconv-win-build
https://github.com/kiyolee/zlib-win-build

and here's my appveyor file:

https://github.com/jamovi/jamovi-readstat/blob/master/appveyor.yml

jonathon-love · 2020-09-09T23:26:07Z

can you check the last update on VS2014?

it all builds for me!

zebrys · 2020-09-10T09:30:33Z

Thank you for this link. I succeed to build a 32-bit version of ReadStat on Appveyor with some fix related to iconv. That being said, the package GNUWin32 is only for 32-bits machines and does not provide 64-bits libs (or static versions as well as debug ones). The unofficial repository https://github.com/pffang/libiconv-for-Windows.git does provides the required additional compiled versions for a proper x64 build. With this, the build succeed.

evanmiller · 2020-09-10T14:33:34Z

@zebrys While you are working on Windows support, you may also want to take a look at issue #200

…t into msvc_support_experimental

zebrys · 2020-09-10T16:26:48Z

This last version should be complete. The CI on MSVC/Windows seems to work correctly. The project has been tested successfully on multiple machine with MSVC 15 and 17!

PS: @evanmiller Ok.

jonathon-love · 2020-09-11T00:43:44Z

oh, sorry evan, we assumed #200 was by design ... we would have submitted a PR long ago if we'd realised you were interested.

i'm sure you'll figure it out @zebrys, but it's just a simple substitution of open() for _wsopen()

here's the implementation jamovi-readstat and pyreadstat use:

https://github.com/jamovi/jamovi-readstat/blob/master/jamovi/readstat.pyx#L26-L41

(in cython).

cheers

zebrys · 2020-09-11T07:38:12Z

The CI seems to fail due to the MSVC configuration and not the branch as the same CI type work on the same branch content. Although I think MSVC 15 should be used regarding the documentation, the actual build version is 16.7 on our CI and 14.1 on your and it seems to prevent the build...

zebrys · 2020-09-11T09:10:59Z

The problem seems to come from the MSVC SDK compatibility. Our CI was using MSVC 2019 by default with the specified OS (despite the Appveyor documentation). The 4 last commits try to enforce MSVC 2015 (even with MSVC 2017/2019 using an older/legacy compiler). Although we try our best to fix the compatibility issue, but we did not succeed so far. Using explicitly MSVC 2017 or 2019 works well in the CI. What about using MSVC 2017 by default for the CI (at least temporary)?

zebrys · 2020-09-11T09:48:09Z

The last commits solve the SDK compatibility. However, this is far from being ideal as a very old SDK has to be used with the current available VS2015 (see the message before).
Thus, a choice should be made between merging this PR as is (with MSVC 2015 enforced and a old SDK) or using a newer version of MSVC 2017 without enforcing the compiler or the SDK.

evanmiller · 2020-09-11T12:57:18Z

Generally I prefer newer tooling, and as development goes forward with the new CI configuration, I'd rather not spend my time chasing down errors with old SDKs.

Since the code base now builds with MSVC 14, I'm okay with using a newer compiler for CI and just waiting for user bug reports about MSVC 14 if it breaks in the future. Is that okay with you, @jonathon-love ?

jonathon-love · 2020-09-11T23:01:28Z

yeah, that's fine. (MSVC 14 is MSVC 2017 i think).

cheers

jonathon

evanmiller · 2020-09-12T00:43:50Z

@zebrys Go ahead and configure it whichever way is most convenient for your team. When you give the word, I'll merge it in.

zebrys · 2020-09-15T07:01:41Z

Tank you. You can now merge this PR!

evanmiller · 2020-09-15T12:08:15Z

Thank you very much for all the work that went into this!

Jogo27 and others added 6 commits November 29, 2018 12:34

Make it compile with GCC 8.2

58f8f72

Visual Studio 2017 compilation process

f4ce478

Merge commit 'f4ce478' into fg-vs17

be94cfe

# Conflicts: # .gitignore

Visual Studio 2017 compilation process

a4b2bf4

RPP-9 #comment Windows compilation process (WinPlatform=10.0.17763.0)

e83e3c0

Merge remote-tracking branch 'origin_root/master' into jr_gcc10_fix

85ff4e7

zebrys changed the title ~~MSVC support~~ MSVC/Windows support Sep 7, 2020

Remove MSVC preprocessor conditionals and index shift, and use malloc…

75727c0

…/realloc for all archs

evanmiller reviewed Sep 8, 2020

View reviewed changes

Fix calloc, free and Windows-based code

c67721f

Move the version in the MSVC project file

9d29cbd

Jérôme Richard and others added 8 commits September 9, 2020 10:39

Update the file list for MSVC and Fix the use of the restrict keyword

7b9ebea

Fix bad VS path

77b47ef

Fix UNIX/POSIX-specific header includes

09b8514

Add READSTAT_VERSION to the two project (wip)

a17cf7b

Fix VLA

7255660

Fix dangerous unsigned type promotion (MSVC error)

cef16ed

Fix relative paths in output obj files for MSVC and add missing tests

65a15df

Fix MSVC build and add all tests

2305fa8

zebrys force-pushed the msvc_support branch from 8822c75 to 2305fa8 Compare September 9, 2020 16:34

Try of MSVC build in the CI

24be3d5

github@rplusplus.com and others added 5 commits September 10, 2020 16:59

Try of MSVC build in the CI (WIP v2)

3fbb6a7

Uniformise VS solutions

c10f588

Fix CI (debug mode)

b070833

Merge branch 'msvc_support_experimental' of github.com:zebrys/ReadSta…

2b46120

…t into msvc_support_experimental

Fix CI (debug mode)

058fd54

evanmiller changed the base branch from master to dev September 11, 2020 11:44

zebrys force-pushed the msvc_support branch from 08d4c2d to 49cc0da Compare September 14, 2020 13:21

Use MSVC 2017 in the CI and execute the msvc toolchain first

b590396

zebrys force-pushed the msvc_support branch from 49cc0da to b590396 Compare September 15, 2020 08:23

evanmiller merged commit d465d52 into WizardMac:dev Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSVC/Windows support #214

MSVC/Windows support #214

zebrys commented Sep 7, 2020

evanmiller commented Sep 7, 2020

evanmiller left a comment

evanmiller Sep 8, 2020

evanmiller Sep 8, 2020

evanmiller Sep 8, 2020

evanmiller Sep 8, 2020

evanmiller Sep 8, 2020

evanmiller Sep 8, 2020

evanmiller commented Sep 8, 2020

zebrys commented Sep 8, 2020

evanmiller commented Sep 8, 2020

jonathon-love commented Sep 8, 2020

jonathon-love commented Sep 8, 2020

evanmiller commented Sep 8, 2020

zebrys commented Sep 9, 2020

zebrys commented Sep 9, 2020

evanmiller commented Sep 9, 2020

zebrys commented Sep 9, 2020

evanmiller commented Sep 9, 2020

jonathon-love commented Sep 9, 2020

jonathon-love commented Sep 9, 2020

zebrys commented Sep 10, 2020

evanmiller commented Sep 10, 2020

zebrys commented Sep 10, 2020 •

edited

Loading

jonathon-love commented Sep 11, 2020

zebrys commented Sep 11, 2020 •

edited

Loading

zebrys commented Sep 11, 2020 •

edited

Loading

zebrys commented Sep 11, 2020

evanmiller commented Sep 11, 2020

jonathon-love commented Sep 11, 2020

evanmiller commented Sep 12, 2020

zebrys commented Sep 15, 2020

evanmiller commented Sep 15, 2020

MSVC/Windows support #214

MSVC/Windows support #214

Conversation

zebrys commented Sep 7, 2020

evanmiller commented Sep 7, 2020

evanmiller left a comment

Choose a reason for hiding this comment

evanmiller Sep 8, 2020

Choose a reason for hiding this comment

evanmiller Sep 8, 2020

Choose a reason for hiding this comment

evanmiller Sep 8, 2020

Choose a reason for hiding this comment

evanmiller Sep 8, 2020

Choose a reason for hiding this comment

evanmiller Sep 8, 2020

Choose a reason for hiding this comment

evanmiller Sep 8, 2020

Choose a reason for hiding this comment

evanmiller commented Sep 8, 2020

zebrys commented Sep 8, 2020

evanmiller commented Sep 8, 2020

jonathon-love commented Sep 8, 2020

jonathon-love commented Sep 8, 2020

evanmiller commented Sep 8, 2020

zebrys commented Sep 9, 2020

zebrys commented Sep 9, 2020

evanmiller commented Sep 9, 2020

zebrys commented Sep 9, 2020

evanmiller commented Sep 9, 2020

jonathon-love commented Sep 9, 2020

jonathon-love commented Sep 9, 2020

zebrys commented Sep 10, 2020

evanmiller commented Sep 10, 2020

zebrys commented Sep 10, 2020 • edited Loading

jonathon-love commented Sep 11, 2020

zebrys commented Sep 11, 2020 • edited Loading

zebrys commented Sep 11, 2020 • edited Loading

zebrys commented Sep 11, 2020

evanmiller commented Sep 11, 2020

jonathon-love commented Sep 11, 2020

evanmiller commented Sep 12, 2020

zebrys commented Sep 15, 2020

evanmiller commented Sep 15, 2020

zebrys commented Sep 10, 2020 •

edited

Loading

zebrys commented Sep 11, 2020 •

edited

Loading

zebrys commented Sep 11, 2020 •

edited

Loading