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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Green build 馃 #322

Merged
merged 10 commits into from Aug 22, 2018

Conversation

Projects
None yet
3 participants
@dkouzoup
Member

dkouzoup commented Aug 21, 2018

  • updated to latest blasfeo/hpipm
  • building shared libraries separately not to have any licensing issues (see #320)
  • temporarily disabled one travis command, that should be restored before merging

Let's see what appveyor says ;)

@dkouzoup dkouzoup changed the title from [WIP] Trying to fix travis to [WIP] Trying to fix appveyor Aug 21, 2018

- export CXX="$CXX -std=c++11" # needed for octave
- cmake --build build
- cmake -E chdir build ctest --output-on-failure
- cmake --build build --target install
- python -c "import acados" # check installation
# - python -c "import acados" # check installation

This comment has been minimized.

@dkouzoup

dkouzoup Aug 21, 2018

Member

@roversch any idea why this fails when BUILD_SHARED_LIBS is ON? The python tests pass without problems.

@dkouzoup

dkouzoup Aug 21, 2018

Member

@roversch any idea why this fails when BUILD_SHARED_LIBS is ON? The python tests pass without problems.

This comment has been minimized.

@roversch

roversch Aug 21, 2018

Contributor

It might be a problem with the acados-SWIG shared lib looking for the other shared libs (acados, blasfeo etc). Before, it was all in one big shared lib, so it didn't have to look for anything (apart from casadi)

@roversch

roversch Aug 21, 2018

Contributor

It might be a problem with the acados-SWIG shared lib looking for the other shared libs (acados, blasfeo etc). Before, it was all in one big shared lib, so it didn't have to look for anything (apart from casadi)

This comment has been minimized.

@dkouzoup

dkouzoup Aug 21, 2018

Member

ok, but if ctest succeeds (also works for me locally), doesn't that mean that the libs are found properly anyway?

@dkouzoup

dkouzoup Aug 21, 2018

Member

ok, but if ctest succeeds (also works for me locally), doesn't that mean that the libs are found properly anyway?

This comment has been minimized.

@dkouzoup

dkouzoup Aug 21, 2018

Member

I guess not all the paths are set properly?

@dkouzoup

dkouzoup Aug 21, 2018

Member

I guess not all the paths are set properly?

This comment has been minimized.

@dkouzoup

dkouzoup Aug 21, 2018

Member

is this check really necessary or shall we merge regardless?

@dkouzoup

dkouzoup Aug 21, 2018

Member

is this check really necessary or shall we merge regardless?

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 21, 2018

Codecov Report

Merging #322 into master will decrease coverage by 2.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
- Coverage   79.19%   77.16%   -2.03%     
==========================================
  Files          65       68       +3     
  Lines       12423    12749     +326     
==========================================
  Hits         9838     9838              
- Misses       2585     2911     +326

codecov bot commented Aug 21, 2018

Codecov Report

Merging #322 into master will decrease coverage by 2.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
- Coverage   79.19%   77.16%   -2.03%     
==========================================
  Files          65       68       +3     
  Lines       12423    12749     +326     
==========================================
  Hits         9838     9838              
- Misses       2585     2911     +326
@dkouzoup

This comment has been minimized.

Show comment
Hide comment
@dkouzoup

dkouzoup Aug 21, 2018

Member

Success! Who's up for ditching this codecov? :P

Member

dkouzoup commented Aug 21, 2018

Success! Who's up for ditching this codecov? :P

@roversch

This comment has been minimized.

Show comment
Hide comment
@roversch

roversch Aug 21, 2018

Contributor

we can ditch it if you want, nobody is using it I guess.

Contributor

roversch commented Aug 21, 2018

we can ditch it if you want, nobody is using it I guess.

@dkouzoup

This comment has been minimized.

Show comment
Hide comment
@dkouzoup

dkouzoup Aug 21, 2018

Member

It's nice to have this percentage actually, but atm I am obsessed with seeing a green light! Is it possible to write the percentage but not giving a red light when the PR reduces the coverage? (btw, I don't even see how my three lines cause 2% reduction)

Member

dkouzoup commented Aug 21, 2018

It's nice to have this percentage actually, but atm I am obsessed with seeing a green light! Is it possible to write the percentage but not giving a red light when the PR reduces the coverage? (btw, I don't even see how my three lines cause 2% reduction)

@dkouzoup dkouzoup referenced this pull request Aug 21, 2018

Closed

Testing PR for codecov #323

@dkouzoup dkouzoup changed the title from [WIP] Trying to fix appveyor to Green build 馃 Aug 21, 2018

@giaf giaf merged commit 9a4bd73 into acados:master Aug 22, 2018

4 checks passed

codecov/patch Coverage not affected when comparing d29772c...06e8dc3
Details
codecov/project 77.16% (target 0%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment