search

mittinatten / freesasa

C-library for calculating Solvent Accessible Surface Areas
http://freesasa.github.io/
MIT License
110 stars 36 forks source link

Windows compatibility #22

Closed mittinatten closed 6 years ago

mittinatten commented 7 years ago

Some C99 and GNU constructs aren't compatible with the MS C compiler. Code should be harmonized. (discussion started at https://github.com/bp-kelley/freesasa/commit/126df0aaaa4c38668acd01b40eff16d1131bb1d1)

Known issues

Other tasks

Work is done in branch windows (https://github.com/mittinatten/freesasa/tree/windows)

mittinatten commented 7 years ago

Ok @bp-kelley, I have removed the VLAs I could find and redefined restrict in the windows-branch. I couldn't find a compiler flag to turn off VLAs without turning off a bunch of other stuff, so there might be a few that I missed. I also added something for _USE_MATH_DEFINES in the affected files.

For restrict I used 

#ifdef _MSC_VER
#define restrict __restrict
#endif

I'm not sure if _MSC_VER is the most appropriate variable to check for?

mittinatten commented 7 years ago

I think I solved the issue with unistd.h in lexer.h in 9b64828d354da9. I just needed to upgrade to a newer version of flex and use the flag --nounistd. The generated files are included in the repo.

bp-kelley commented 7 years ago

That should work fine for all the versions of windows you are likely to see, but for full backwards compatibility you could do

#if defined(_MSC_VER) && _MSC_VER >= 1400
  #define restrict __restrict
#else
  #define restrict
#endif

AS I think __restrict was added in version 1400

bp-kelley commented 7 years ago

@mittinatten let me know when you want me to try a windows build. I'll pull this into my fork and add an appveyor windows build script.

mittinatten commented 7 years ago

Hi, I have added all the modifications I am aware of are necessary in the branch 'windows'. There is one spurious error that occured today in one of the tests but didn't yesterday, so I guess it's a buffer overflow or something that by chance didn't have any consequences yesterday, but I think I can work on that in parallel. So whenever you have time to look at AppVeyor integration, just go ahead!

Simon

mittinatten commented 7 years ago

Hi again @bp-kelley, I solved the spurious error, it wasn't related to any of this, but a test that was too big and timed out. My computer is simply getting slower. I pushed a fix to dev.

bp-kelley commented 7 years ago

Great! Here is my current plan:

  1. Work on getting the rdkit integration with FreeSASA working again and tested on Linux/OS X/windows
  2. With your permission, I'll either add a visual studio project file or a cmake make file to build on windows, this will require a getline implementation which may take a little while.

Brian Kelley

On Jun 8, 2017, at 2:08 PM, Simon Mitternacht notifications@github.com wrote:

Hi again, I solved the spurious error, it wasn't related to any of this, but a test that was too big and timed out. My computer is simply getting slower. I pushed a fix to dev.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mittinatten commented 7 years ago

Sounds good!

  1. Just let me know if there are any problems
  2. Sounds fine, as long as the footprint can be kept small: If possible have configuration scripts write to config.h, and reuse as many of the flags there as possible, etc. (i.e. avoid if (USE_JSON || MSC_USE_JSON) ...). Getline is a problem, and I was hesitant to use it to begin with (but it's just very convenient). A custom one would have to be conditionally linked, as I'd prefer to keep the GNU implementation when available.

I'm soon heading for vacation, but will be back in July, just so you know why correspondence might be a bit sparse.

bp-kelley commented 7 years ago

Thanks for the reply. I'll work on the RDKit integration first and then extend. I'm also out until July as well.

On Fri, Jun 9, 2017 at 3:45 AM, Simon Mitternacht notifications@github.com wrote:

Sounds good!

  1. Just let me know if there are any problems
  2. Sounds fine, as long as the footprint can be kept small: If possible have configuration scripts write to config.h, and reuse as many of the flags there as possible, etc. (i.e. avoid if (USE_JSON || MSC_USE_JSON) ...). Getline is a problem, and I was hesitant to use it to begin with (but it's just very convenient). A custom one would have to be conditionally linked, as I'd prefer to keep the GNU implementation when available.

I'm soon heading for vacation, but will be back in July, just so you know why correspondence might be a bit sparse.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mittinatten/freesasa/issues/22#issuecomment-307320078, or mute the thread https://github.com/notifications/unsubscribe-auth/AJbioEYJntzWqY6mqjvg0prsm1UQdzysks5sCPgbgaJpZM4NwYs7 .

mittinatten commented 7 years ago

Progress report: I replaced getline with fgets everywhere. This was very straightforward. The main consequence was that we now have a max line length for input PDB and classifier files. PDB files have a natural limitation, I added some extra line length to allow non-standard input. Classifier files could possibly have comments that are too long, this should be documented and if it causes any errors these should give clear messages.

I also want to add some more stress tests to classifier reading without VLAs.

mittinatten commented 7 years ago

Now all VLAs are gone too, i.e. it compiles with -Werror=vla. (The commit mess above is me forgetting to amend and then squashing, so 8dcf2f2 and 6a23f7c can be ignored).

mittinatten commented 7 years ago

I have addressed the remaining issues above too, but can't verify that the solutions work since I don't have a Windows machine. Can you check when you have a chance @bp-kelley?

Before we call that part a wrap I want to stress-test the modified classifier.c a bit more too, as mentioned, since I introduced some larger changes there and I think there might be new edge cases to check.

bp-kelley commented 7 years ago

Absolutely. We released the Linux rdkit bindings, I'll send you a link when I get back to civilization in a few days.

Brian Kelley

On Sep 30, 2017, at 2:37 PM, Simon Mitternacht notifications@github.com wrote:

I have addressed the remaining issues above too, but can't verify that the solutions work since I don't have a Windows machine. Can you check when you have a chance @bp-kelley?

Before we call that part a wrap I want to stress-test the modified classifier.c a bit more too, as mentioned, since I introduced some larger changes there and I think there might be new edge cases to check.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mittinatten commented 7 years ago

Hi again @bp-kelley, any news here?

I would like to wrap up this part pretty soon. There are a few tiny changes and bug fixes that I'd like to get published as 2.0.2, including some of the changes done for this issue. We can save adding MS project files for 2.0.3. So if you could verify that the windows branch compiles with the MS C compiler, we're all set for a release.

bp-kelley commented 7 years ago

Simon, I'll start a new build this evening and let you know what happens. Thanks for taking the time to do this btw!

Brian Kelley

On Oct 15, 2017, at 5:19 AM, Simon Mitternacht notifications@github.com wrote:

Hi again @bp-kelley, any news here?

I would like to wrap up this part pretty soon. There are a few tiny changes and bug fixes that I'd like to get published as 2.0.2, including some of the changes done for this issue. We can save adding MS project files for 2.0.3. So if you could verify that the windows branch compiles with the MS C compiler, we're all set for a release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mittinatten commented 7 years ago

Great! Just let me know if there's any trouble.

bp-kelley commented 7 years ago

It looks good so far. I'll need to run the regression tests, but the initial builds went fine.

On Mon, Oct 16, 2017 at 7:53 AM, Simon Mitternacht <notifications@github.com

wrote:

Great! Just let me know if there's any trouble.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mittinatten/freesasa/issues/22#issuecomment-336864118, or mute the thread https://github.com/notifications/unsubscribe-auth/AJbioLJQ8XIRzoMDEW8UUMWdWwxx-LK6ks5ss0PKgaJpZM4NwYs7 .

mittinatten commented 7 years ago

Good to hear, thank you for doing this!

Do you mean RDKit tests or the ones that ship with FreeSASA?

bp-kelley commented 7 years ago

RDKits. We use appveyor for the builds, but they take too long to run the regression tests automatically.

Brian Kelley

On Oct 17, 2017, at 9:21 AM, Simon Mitternacht notifications@github.com wrote:

Good to hear, thank you for doing this!

Do you mean RDKit tests or the ones that ship with FreeSASA?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mittinatten commented 6 years ago

Closing this issue for milestone/release purposes. Can create a new one if necessary when we start with the project files.

mittinatten commented 6 years ago

Here's the tag https://github.com/mittinatten/freesasa/releases/tag/2.0.2

mittinatten commented 6 years ago

In the process of creating a PyPi-package I needed to deploy Windows binaries for the Python module. I therefore made the parts of the library that are required by the Python bindings C89-compatible, given the right config.h and/or compiler flags.

This means that everything except main.c, xml.c and json.c in version 2.0.3 builds correctly with a number of versions of MSVC (as long as USE_XML=0, USE_JSON=0 and USE_CHECK=0). The three source files can probably be excluded in most use cases for the C API, the XML and JSON options are mainly intended for the CLI, and being able to compile the unit tests isn't necessary when using FreeSASA as a library.

The Python module is now in a separate repo: https://github.com/freesasa/freesasa-python, which has an Appveyor setup for CI/CD for several version of Python in Windows (and Travis for Mac OS). The config there should be easy to copy for those who want to build FreeSASA as a C library on Windows, i.e. compile the the same files using the same config.h. Since different Python versions are associated with different MSVC versions, this setup verifies that the library compiles with all those compiler versions.

For the CLI to build with MSVC, one will need to find a replacement for getopt_long().