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

Build fails with GCC 8 #1299

Closed
AdamWill opened this issue Mar 8, 2018 · 7 comments · Fixed by #1302
Closed

Build fails with GCC 8 #1299

AdamWill opened this issue Mar 8, 2018 · 7 comments · Fixed by #1302

Comments

@AdamWill
Copy link

AdamWill commented Mar 8, 2018

Build of mlpack fails with GCC 8:

cd /builddir/build/BUILD/mlpack-2.2.5/src/mlpack/methods/logistic_regression && /usr/bin/c++  -DARMA_NO_DEBUG -DBOOST_TEST_DYN_LINK -DHAS_OPENMP -DNDEBUG -I/builddir/build/BUILD/mlpack-2.2.5 -I/builddir/build/BUILD/mlpack-2.2.5/src/mlpack/..  -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -mcet -fcf-protection -Wall -Wextra -ftemplate-depth=1000 -O3 -fopenmp   -std=gnu++11 -o CMakeFiles/mlpack_logistic_regression.dir/logistic_regression_main.cpp.o -c /builddir/build/BUILD/mlpack-2.2.5/src/mlpack/methods/logistic_regression/logistic_regression_main.cpp
In file included from /builddir/build/BUILD/mlpack-2.2.5/src/mlpack/methods/logistic_regression/logistic_regression.hpp:19,
                 from /builddir/build/BUILD/mlpack-2.2.5/src/mlpack/methods/logistic_regression/logistic_regression_main.cpp:16:
/builddir/build/BUILD/mlpack-2.2.5/src/mlpack/methods/logistic_regression/logistic_regression_function.hpp: In member function 'const vec& mlpack::regression::LogisticRegressionFunction<MatType>::Responses() const':
/builddir/build/BUILD/mlpack-2.2.5/src/mlpack/methods/logistic_regression/logistic_regression_function.hpp:53:47: error: invalid initialization of reference of type 'const vec&' {aka 'const arma::Col<double>&'} from expression of type 'const arma::Row<long unsigned int>'
   const arma::vec& Responses() const { return responses; }

Basically, it doesn't like this:

 const arma::Row<size_t>& responses,

...

 const arma::vec& Responses() const { return responses; }

I think.

@zoq
Copy link
Member

zoq commented Mar 9, 2018

Interesting, we are testing against different llvm and gcc versions:

but not sure if anyone tested gcc 8 since it has not been released yet.

Do you mind to provide a minimal code example? Would be helpful to narrow it down.

@rcurtin
Copy link
Member

rcurtin commented Mar 9, 2018

@AdamWill: thanks for the report. It looks like the issue is just that the arma::vec& in the definition of Responses() should be changed to arma::Row<size_t>... oops.

I'll open a simple PR to fix this in the master branch, which will have a 3.0.0 release very shortly. So when that release happens there will be no issue. However, that does not fix your problem for mlpack 2.2.5 (and I'd rather avoid the effort of releasing 2.2.6, because our process for a release is a bit time-consuming). So, I can provide a simple patch that you can apply in the spec file for the Fedora package, if that works for you.

@rcurtin
Copy link
Member

rcurtin commented Mar 12, 2018

This is fixed with #1302, but it will be a little while until an mlpack 3 release occurs with the fix in it. If a patch is needed for mlpack 2.2.5 just let me know.

@AdamWill
Copy link
Author

That's fine, we don't have any trouble backporting patches. Thanks.

@rcurtin
Copy link
Member

rcurtin commented Mar 12, 2018

Ok, I take that to mean you'll handle any patching then. If that's not correct just let me know. Otherwise, thanks for the bug report and I am glad to have the issue fixed. :)

@AdamWill
Copy link
Author

that's right. it's a highly involved process wherein we do wget https://github.com/mlpack/mlpack/pull/1302.patch and add it to the package spec. :P

@rcurtin
Copy link
Member

rcurtin commented Mar 12, 2018

:) Just checking.

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

Successfully merging a pull request may close this issue.

3 participants