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

Build tests on appveyor, the MSVC build, but don't run them yet. #556

Merged
merged 1 commit into from Jun 5, 2017

Conversation

Projects
None yet
4 participants
@iphydf
Member

iphydf commented Jun 4, 2017


This change is Reviewable

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Jun 4, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


.travis.yml, line 3 at r1 (raw file):

language: c

# TODO(iphydf): Uncomment this before submitting.

It seems you forgot this ;)


Comments from Reviewable

Diadlo commented Jun 4, 2017

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


.travis.yml, line 3 at r1 (raw file):

language: c

# TODO(iphydf): Uncomment this before submitting.

It seems you forgot this ;)


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Jun 4, 2017

Member

@Diadlo I'm still working on it.

Member

iphydf commented Jun 4, 2017

@Diadlo I'm still working on it.

@iphydf iphydf changed the title from Run tests on appveyor, the MSVC build. to WIP: Run tests on appveyor, the MSVC build. Jun 4, 2017

@iphydf iphydf changed the title from WIP: Run tests on appveyor, the MSVC build. to Build tests on appveyor, the MSVC build, but don't run them yet. Jun 4, 2017

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Jun 5, 2017

Member

This is now ready for review.

Member

iphydf commented Jun 5, 2017

This is now ready for review.

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Jun 5, 2017

Member

Review status: 0 of 26 files reviewed at latest revision, 1 unresolved discussion.


.travis.yml, line 3 at r1 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

It seems you forgot this ;)

Done.


Comments from Reviewable

Member

iphydf commented Jun 5, 2017

Review status: 0 of 26 files reviewed at latest revision, 1 unresolved discussion.


.travis.yml, line 3 at r1 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

It seems you forgot this ;)

Done.


Comments from Reviewable

@iphydf iphydf added this to the v0.1.9 milestone Jun 5, 2017

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Jun 5, 2017

Reviewed 28 of 28 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


appveyor.yml, line 11 at r2 (raw file):

- if not exist %APPDATA%\downloads mkdir %APPDATA%\downloads
# libcheck
- if not exist %APPDATA%\downloads\check.tar.gz curl -L https://github.com/libcheck/check/archive/0.11.0.zip -o %APPDATA%\downloads\check.zip

You check the existence of check.tar.gz but download check.zip


cmake/Dependencies.cmake, line 69 at r2 (raw file):

  find_library(LIBCHECK_LIBRARIES
    NAMES check libcheck
    PATHS "C:/Program Files (x86)/check/lib"

I think %ProgramFilesDir%/check/lib will be better. Same for other "Prgram Files (x86)"


Comments from Reviewable

Diadlo commented Jun 5, 2017

Reviewed 28 of 28 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


appveyor.yml, line 11 at r2 (raw file):

- if not exist %APPDATA%\downloads mkdir %APPDATA%\downloads
# libcheck
- if not exist %APPDATA%\downloads\check.tar.gz curl -L https://github.com/libcheck/check/archive/0.11.0.zip -o %APPDATA%\downloads\check.zip

You check the existence of check.tar.gz but download check.zip


cmake/Dependencies.cmake, line 69 at r2 (raw file):

  find_library(LIBCHECK_LIBRARIES
    NAMES check libcheck
    PATHS "C:/Program Files (x86)/check/lib"

I think %ProgramFilesDir%/check/lib will be better. Same for other "Prgram Files (x86)"


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Jun 5, 2017

Member

Review status: 25 of 27 files reviewed at latest revision, 2 unresolved discussions.


appveyor.yml, line 11 at r2 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

You checke the existence of check.tar.gz but download check.zip

Done.


cmake/Dependencies.cmake, line 69 at r2 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

I think %ProgramFilesDir%/check/lib will be better. Same for other "Prgram Files (x86)"

Done.


Comments from Reviewable

Member

iphydf commented Jun 5, 2017

Review status: 25 of 27 files reviewed at latest revision, 2 unresolved discussions.


appveyor.yml, line 11 at r2 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

You checke the existence of check.tar.gz but download check.zip

Done.


cmake/Dependencies.cmake, line 69 at r2 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

I think %ProgramFilesDir%/check/lib will be better. Same for other "Prgram Files (x86)"

Done.


Comments from Reviewable

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Jun 5, 2017

Member

CMakeLists.txt, line 397 at r2 (raw file):

      set_tests_properties(${target} PROPERTIES TIMEOUT "${TEST_TIMEOUT_SECONDS}")
    endif()
  endif()

Kind of weird how DISABLED test means "build=true, run=false", while MSVC_DISABLED means "build=false, run=false". One could think that MSVC_DISABLED does exactly what DISABLED does but only for MSVC and they would be wrong.

Perhaps changing DISABLED to DONT_RUN and MSVC_DISABLED to MSVC_DONT_BUILD, or to something along these lines, would be better? Alternatively, keeping DISABLED as it is but removing MSVC_DISABLED by pulling the if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") out of auto_test() would be fine too.


Comments from Reviewable

Member

nurupo commented Jun 5, 2017

CMakeLists.txt, line 397 at r2 (raw file):

      set_tests_properties(${target} PROPERTIES TIMEOUT "${TEST_TIMEOUT_SECONDS}")
    endif()
  endif()

Kind of weird how DISABLED test means "build=true, run=false", while MSVC_DISABLED means "build=false, run=false". One could think that MSVC_DISABLED does exactly what DISABLED does but only for MSVC and they would be wrong.

Perhaps changing DISABLED to DONT_RUN and MSVC_DISABLED to MSVC_DONT_BUILD, or to something along these lines, would be better? Alternatively, keeping DISABLED as it is but removing MSVC_DISABLED by pulling the if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") out of auto_test() would be fine too.


Comments from Reviewable

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Jun 5, 2017

Member

cmake/Dependencies.cmake, line 69 at r2 (raw file):

Previously, iphydf wrote…

Done.

I don't understand what was wrong with using the working directory for everything and doing things relative to its path. You don't install libraries in Program Files on Windows. When you develop on Windows, all libraries your program depends on are usually in some directory called "deps" or "3rd-party" next to your source tree. You also ship the libraries together with your software product, so that when you install, say, qTox, all the libraries qTox depends on are in C:/Program Files (x86)/qTox, next to qTox.exe, not in C:/Program Files (x86)/tocore, C:/Program Files (x86)/opus, C:/Program Files (x86)/sodium, etc.


Comments from Reviewable

Member

nurupo commented Jun 5, 2017

cmake/Dependencies.cmake, line 69 at r2 (raw file):

Previously, iphydf wrote…

Done.

I don't understand what was wrong with using the working directory for everything and doing things relative to its path. You don't install libraries in Program Files on Windows. When you develop on Windows, all libraries your program depends on are usually in some directory called "deps" or "3rd-party" next to your source tree. You also ship the libraries together with your software product, so that when you install, say, qTox, all the libraries qTox depends on are in C:/Program Files (x86)/qTox, next to qTox.exe, not in C:/Program Files (x86)/tocore, C:/Program Files (x86)/opus, C:/Program Files (x86)/sodium, etc.


Comments from Reviewable

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Jun 5, 2017

Member

Reviewed 28 of 28 files at r2.
Review status: 25 of 27 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

Member

nurupo commented Jun 5, 2017

Reviewed 28 of 28 files at r2.
Review status: 25 of 27 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Jun 5, 2017

Member

Review status: 24 of 27 files reviewed at latest revision, 3 unresolved discussions.


CMakeLists.txt, line 397 at r2 (raw file):

Previously, nurupo wrote…

Kind of weird how DISABLED test means "build=true, run=false", while MSVC_DISABLED means "build=false, run=false". One could think that MSVC_DISABLED does exactly what DISABLED does but only for MSVC and they would be wrong.

Perhaps changing DISABLED to DONT_RUN and MSVC_DISABLED to MSVC_DONT_BUILD, or to something along these lines, would be better? Alternatively, keeping DISABLED as it is but removing MSVC_DISABLED by pulling the if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") out of auto_test() would be fine too.

Done.


cmake/Dependencies.cmake, line 69 at r2 (raw file):

Previously, nurupo wrote…

I don't understand what was wrong with using the working directory for everything and doing things relative to its path. You don't install libraries in Program Files on Windows. When you develop on Windows, all libraries your program depends on are usually in some directory called "deps" or "3rd-party" next to your source tree. You also ship the libraries together with your software product, so that when you install, say, qTox, all the libraries qTox depends on are in C:/Program Files (x86)/qTox, next to qTox.exe, not in C:/Program Files (x86)/tocore, C:/Program Files (x86)/opus, C:/Program Files (x86)/sodium, etc.

Added an explanation for it. I like to override defaults as little as possible.


Comments from Reviewable

Member

iphydf commented Jun 5, 2017

Review status: 24 of 27 files reviewed at latest revision, 3 unresolved discussions.


CMakeLists.txt, line 397 at r2 (raw file):

Previously, nurupo wrote…

Kind of weird how DISABLED test means "build=true, run=false", while MSVC_DISABLED means "build=false, run=false". One could think that MSVC_DISABLED does exactly what DISABLED does but only for MSVC and they would be wrong.

Perhaps changing DISABLED to DONT_RUN and MSVC_DISABLED to MSVC_DONT_BUILD, or to something along these lines, would be better? Alternatively, keeping DISABLED as it is but removing MSVC_DISABLED by pulling the if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC") out of auto_test() would be fine too.

Done.


cmake/Dependencies.cmake, line 69 at r2 (raw file):

Previously, nurupo wrote…

I don't understand what was wrong with using the working directory for everything and doing things relative to its path. You don't install libraries in Program Files on Windows. When you develop on Windows, all libraries your program depends on are usually in some directory called "deps" or "3rd-party" next to your source tree. You also ship the libraries together with your software product, so that when you install, say, qTox, all the libraries qTox depends on are in C:/Program Files (x86)/qTox, next to qTox.exe, not in C:/Program Files (x86)/tocore, C:/Program Files (x86)/opus, C:/Program Files (x86)/sodium, etc.

Added an explanation for it. I like to override defaults as little as possible.


Comments from Reviewable

Build tests on appveyor, the MSVC build.
Tests are not actually ran on appveyor for now, since they all fault for
some reason. For now, we just build them. Also, some tests are disabled
on msvc entirely, because they don't even compile. We'll need to look
into those, later. They are disabled using `MSVC_DONT_BUILD`.
@robinlinden

This comment has been minimized.

Show comment
Hide comment
@robinlinden

robinlinden Jun 5, 2017

Member

:lgtm_strong:


Reviewed 22 of 28 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

Member

robinlinden commented Jun 5, 2017

:lgtm_strong:


Reviewed 22 of 28 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@iphydf iphydf merged commit cb69b8a into TokTok:master Jun 5, 2017

5 of 8 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
code-review/reviewable 1/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 74.292%
Details
license/cla Contributor License Agreement is signed.
Details

@iphydf iphydf deleted the iphydf:msvc-tests branch Jun 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment