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

Compilation in Windows (MSVC2015) #115

Closed
wants to merge 15 commits into from
Closed

Conversation

jgsogo
Copy link

@jgsogo jgsogo commented Oct 19, 2015

This PR gathers commits in order to compile MeTA in MSVC2015. It is a work in progress, so this message may be edited if neccesary.

It also depends in modifications/PR of other libraries:

@jgsogo
Copy link
Author

jgsogo commented Oct 19, 2015

Next issue to be solved (ideas?):

Fails on project meta-classify when trying to compile external projects added at ~/src/classify/CMakeLists.txt due to invalid BUILD_COMMAND

Now it is

BUILD_COMMAND CXX=${CMAKE_CXX_COMPILER} CC=${CMAKE_C_COMPILER} make

which generates the following command

set(command "CXX=E:/ProgramFiles/MSVS2015/VC/bin/x86_amd64/cl.exe;CC=E:/ProgramFiles/MSVS2015/VC/bin/x86_amd64/cl.exe;make")

and it fails when executed in CMD as CXX is an unrecognized command

@skystrife
Copy link
Member

Ah, yeah, the libsvm modules... (ugh).

Let me see if I can convert those to use cmake themselves, which should make it easier...

@skystrife
Copy link
Member

OK, can you try merging (or rebasing) in the latest commits from develop? I think 63401b7 should help here by making the libsvm modules be CMake projects themselves, instead of just hackily calling make and assuming everything will just work.

@jgsogo
Copy link
Author

jgsogo commented Oct 20, 2015

I've rebased and push changes following your comments in bb1b547 It compiles (libsvm modules too) ;D

Pending:

@jgsogo
Copy link
Author

jgsogo commented Oct 20, 2015

Here it is the output with all the warnings: https://gist.github.com/jgsogo/b81bbd7210b0215d91a5

@jgsogo
Copy link
Author

jgsogo commented Oct 20, 2015

@skystrife Shall I address some of those warnings? I've seen that you have several commits in branch appveyor-ci which will solve some of them. Maybe the way to go is to merge that branch with this PR.

@skystrife
Copy link
Member

Looks like the build is failing because of the submodule commit hash for porter2_stemmer. Can you update it to the tip of master, which is 52c6b5c00c7e38a47013c55f0cc3cda7d5a48dca?

@skystrife
Copy link
Member

As far as the warnings go, I'd like to fix them for sure but I wouldn't block merging this on them.

Were you able to run the unit tests on the MSVC build?

@jgsogo
Copy link
Author

jgsogo commented Oct 21, 2015

Unit tests fail because unit-test.exe cannot find icuin55.dll, icuuc55.dll and icudt55.dll in the system. I've copied them manually to the unit-test folder so I can run tests. This is the output for the tests:

Failed

  • analyzers

    content-unigram-word-analyzer:          config.toml could not be opened for parsing
    content-bigram-word-analyzer:           config.toml could not be opened for parsing
    content-trigram-word-analyzer:          config.toml could not be opened for parsing
    file-unigram-word-analyzer:             config.toml could not be opened for parsing
    file-bigram-word-analyzer:              config.toml could not be opened for parsing
    file-trigram-word-analyzer:             config.toml could not be opened for parsing
    
  • vocabulary-map: Timeout

    vocabulary_writer_full_block:           [ ←[31mFAIL←[0m ] [term == expected[idx].first] => [ == b] (vocabulary_map_test.cpp:83)
    vocabulary_writer_partial_blocks:       [ ←[31mFAIL←[0m ] [term == expected[idx].first] => [ == b] (vocabulary_map_test.cpp:83)
    
  • ir-eval

    ir-eval-bounds:                         config.toml could not be opened for parsing
    ir-eval-results:                        config.toml could not be opened for parsing
    
  • Others fail with Debug error! abort() has been called:

    • inverted-index
    • forward-index
    • classifiers
    • rankers
    • language-model
    • features

Passed

  • stemmers
  • parallel
  • string-list
  • libsvm-parser
  • compression
  • graph
  • parser
  • filesystem

I have no time now to investigate further (although I'll try to)

@smassung
Copy link
Member

Where is unit-test.exe in relation to the config file? I'm guessing the current directory while running unit tests is different on Windows than on the other systems. I think the Debug error! abort() tests that failed also have to do with the config file, since the tests that passed do not use config files.

@jgsogo
Copy link
Author

jgsogo commented Oct 21, 2015

unit-test.exe is generated in <my-path>\meta\build_2015\Debug (I told CMake to build the binaries in <my-path>/meta/build_2015), so

  • icu...dll files should be copied to that path too, and
  • config.toml must be in <my-path>\build_2015\src\test\tools\config.toml (when executed the project unit-test from inside Visual Studio).... but I think that this is not needed if you run RUN_TESTS (which is what should be done)

@jgsogo
Copy link
Author

jgsogo commented Oct 21, 2015

Nevertheless there is an assert (string iterator not dereferencable) in cpptoml.h#L1705

if (!is_exp && (*check_it == 'e' || *check_it == 'E'))

at this point check_it is empty (it was 1.2) at the beginning of the function.


Maybe it is interesting to have some tests associated with this set of functions...

@skystrife
Copy link
Member

This is a bug upstream. (Upstream just so happens to be me, so I'll look into fixing it.)

cpptoml is well tested (with sanitizers, too). I think this is innocuous (the iterator points to the null terminator, which is valid memory) but that MSVC is technically correct. Let me fix it.

@skystrife
Copy link
Member

OK, try updating the cpptoml submodule to the tip of master and see if that helps. If there are more issues with the config file parsing, go ahead and file issues upstream for them and I can fix them there.

@jgsogo
Copy link
Author

jgsogo commented Nov 22, 2015

Hi! I'm here again, sorry for dissappering, it's been a month full of changes, but now I'm quiet again and willing to contribute.

I've updated cpptoml and previous issue is fixed, but now I come accross an Access violation reading location exception at meta/src/index/vocabulary_map.cpp#L21 here

I see a lot of changes (great!), should I merge/rebase develop branch?

@skystrife
Copy link
Member

Closing for inactivity. Please feel free to re-open if you have more time to dedicate to this.

If someone else wants to take this on, I'd be more than happy to have things building with MSVC. In theory we could then add a job to AppVeyor so that we can keep the build for MSVC working even if no individual contributor is using it on a daily basis.

@skystrife skystrife closed this Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants