Skip to content

Conversation

@soroshsabz
Copy link
Contributor

@soroshsabz soroshsabz commented Apr 1, 2019

ITNOA

Add appveyor configuration for CI on windows :) . (resolve #258 )

@soroshsabz
Copy link
Contributor Author

@fnc12 Please help to resolve errors on windows, I resolved dependency errors on windows, but now we can see some compile errors on

examples\except_intersection.cpp(64)
examples\except_intersection.cpp(65)
examples\except_intersection.cpp(67)
...

examples\union.cpp(76)
examples\union.cpp(81)
examples\union.cpp(82)
...

@fnc12
Copy link
Owner

fnc12 commented Apr 1, 2019

please paste error text here

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 1, 2019

@fnc12 you can see in Details left on appveyor

also for example error:

examples\except_intersection.cpp(64): error C2672: 'sqlite_orm::internal::storage_t<sqlite_orm::internal::table_t<DeptMaster,sqlite_orm::internal::column_t<DeptMast er,int,const int &(__thiscall DeptMaster::* )(void) const,void (__thiscall DeptMaster::* )(int),sqlite_orm::constraints::autoincrement_t,sqlite_orm::constraints::primary_key_t<>>,sqlite_orm::internal::column_t<DeptMaster,std::string,const
std::string &(__thiscall DeptMaster::* )(void) const,void (__thiscall DeptMaster::* )(std::string)>>,sqlite_orm::internal::table_t<EmpMaster,sqlite_orm::internal::column_t<EmpMaster,int,const int &(__thiscall EmpMaster::* )(void) const,voi d (__thiscall EmpMaster::* )(int),sqlite_orm::constraints::autoincrement_t,sqlite_orm::constraints::primary_key_t<>>,sqlite_orm::internal::column_t<EmpMaster,std::string,const std::string &(__thiscall EmpMaster::* )(void) const,void (__thi scall EmpMaster::* )(std::string)>,sqlite_orm::internal::column_t<EmpMaster,std::string,const std::string &(__thiscall EmpMaster::* )(void) const,void (__thiscall EmpMaster::* )(std::string)>,sqlite_orm::internal::column_t<EmpMaster,long,c onst long &(__thiscall EmpMaster::* )(void) const,void (__thiscall EmpMaster::* )(long)>,sqlite_orm::internal::column_t<EmpMaster,int,const int &(__thiscall EmpMaster::* )(void) const,void (__thiscall EmpMaster::* )(int)>>>::select': no ma tching overloaded function found [\temp\examples\except_intersection.vcxproj]

examples\except_intersection.cpp(65): error C2783: 'std::vector<R,std::allocator<R>> sqlite_orm::internal::storage_t<sqlite_orm::internal::table_t<DeptMaster,sqlite _orm::internal::column_t<DeptMaster,int,const int &(__thiscall DeptMaster::* )(void) const,void (__thiscall DeptMaster::* )(int),sqlite_orm::constraints::autoincrement_t,sqlite_orm::constraints::primary_key_t<>>,sqlite_orm::internal::colum n_t<DeptMaster,std::string,const std::string &(__thiscall DeptMaster::* )(void) const,void (__thiscall DeptMaster::* )(std::string)>>,sqlite_orm::internal::table_t<EmpMaster,sqlite_orm::internal::column_t<EmpMaster,int,const int &(__thisca ll EmpMaster::* )(void) const,void (__thiscall EmpMaster::* )(int),sqlite_orm::constraints::autoincrement_t,sqlite_orm::constraints::primary_key_t<>>,sqlite_orm::internal::column_t<EmpMaster,std::string,const std::string &(__thiscall EmpMa ster::* )(void) const,void (__thiscall EmpMaster::* )(std::string)>,sqlite_orm::internal::column_t<EmpMaster,std::string,const std::string &(__thiscall EmpMaster::* )(void) const,void (__thiscall EmpMaster::* )(std::string)>,sqlite_orm::in ternal::column_t<EmpMaster,long,const long &(__thiscall EmpMaster::* )(void) const,void (__thiscall EmpMaster::* )(long)>,sqlite_orm::internal::column_t<EmpMaster,int,const int &(__thiscall EmpMaster::* )(void) const,void (__thiscall EmpMa ster::* )(int)>>>::select(T,Args...)': could not deduce template argument for 'R' [\temp\examples\except_intersection.vcxproj]

Did you have any idea?

@soroshsabz
Copy link
Contributor Author

@fnc12 I found it, this problem because of Bug in MSVC, and workaround about it in stackoverflow

@soroshsabz
Copy link
Contributor Author

@fnc12 please cancel all queue run, and rerun again, I wish to pass test :)

thanks

@soroshsabz
Copy link
Contributor Author

@fnc12 did you can resolve below warnings:

sqlite-orm\tests\tests.cpp(1043): warning C4700: uninitialized local variable 'data' used

if you can, please add changes in this PR

thanks

@soroshsabz
Copy link
Contributor Author

@fnc12 As you can see in appveyor release failed report , project compile correctly but in ctest we can get error, but in debug mode test pass ( in local run ) . so if you have time, please help me to investigate ctest release mode problem in windows.

thanks

@soroshsabz
Copy link
Contributor Author

@fnc12 As you can see in appveyor release failed report , project compile correctly but in ctest we can get error, but in debug mode test pass ( in local run ) . so if you have time, please help me to investigate ctest release mode problem in windows.
thanks

Problem is for testExplicitInsert function, but I don't know why

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 2, 2019

@fnc12 After I some times debug it, I found line of error

            auto insertedUser3 = storage.get<User>(user3.id); // line : 647 -- exception here
            auto visitFromStorage = storage.get<Visit>(visit2.id()); // line : 688 -- another exception here

exception in get function and is

            throw std::system_error(std::make_error_code(sqlite_orm::orm_error_code::not_found));

after this line we catch exception. I hope this information is useful for you to debug it.

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

this is weird. My local run is also successful. Looks like that we insert a used with explicit id and cannot get him on the next line. If there is a way to see cout output could you please add info about all users in the table before the line with exception like this:

for(auto &row : storage.iterate<User>()){
    cout << storage.dump(user) << endl;
}

It will show us all contents of the table before getting any row. Thanks

@soroshsabz
Copy link
Contributor Author

this is weird. My local run is also successful. Looks like that we insert a used with explicit id and cannot get him on the next line. If there is a way to see cout output could you please add info about all users in the table before the line with exception like this:
for(auto &row : storage.iterate()){
cout << storage.dump(user) << endl;
}
It will show us all contents of the table before getting any row. Thanks

It is weird for me too at first, but I found the problem, it is because my wackiness, assert does not run in release mode, so all insert that in assert does not run, so the result of your code is like below

{ id : '0', name : 'Juan', age : '57', email : '' }

just one record printed.

to resolved this issue I have to undef NDEUBG in tests.cpp

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

I think it is better to replace assert with throw in tests code. Defining NDEBUG is not the best solution cause asserts can be useful in other cases but your define will change their behavior in release/debug. Thanks

@soroshsabz
Copy link
Contributor Author

@fnc12 As you can see in last commit, I just undef NDEBUG for cassert header and redefine it for another code and library. so I think it is not a bad solution. Are you disagree with me?

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 2, 2019

Finally test pass in release mode :)

And finally all check pass in PR :)

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

that's cool but let's instead throw an exception instead of asserts. I know that it is my mistake that I wrote asserts instead of exception in the very beginning. I can fix it by myself

@soroshsabz
Copy link
Contributor Author

@fnc12 ok, I suggest resolve #256 is principled solution for this, so I suggest if you want to change assert, change whole of tests to gtest framework and use EXPECT and etc … command with good readability.

thanks :)

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

Commiting #define NDEBUG is not the best practice. Please let's avoid it. Let me replace asserts with throw in dev branch, than pull changes and submit this PR. It is quicker that adding gtest. We will add gtest afterwards and unchain these two tasks

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 2, 2019

@fnc12 if you want I can replace it, but I think undef NDEBUG in tests.cpp is not a bad practice, because does not affect any other area, as you can see in last my commit, and we can use this code, and use appveyor until switching to another solution.

Using undef NDEBUG is common practice to using assert in release mode.

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

define NDEBUG must work in automatic mode not manual. Any compiler must do it for any release build. But undef NDEBUG will change the whole app and we will face this problem once we start using asserts somewhere else. Macro NDEBUG is a cross-app feature and it is a bad practice to make this behavior different in our app.

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 2, 2019

define NDEBUG must work in automatic mode not manual. Any compiler must do it for any release build. But undef NDEBUG will change the whole app and we will face this problem once we start using asserts somewhere else. Macro NDEBUG is a cross-app feature and it is a bad practice to make this behavior different in our app.

Ok, but I change this macro only in tests.cpp and only up of cassert header and back them normally after include, this change does not change any behavior on app or on STL and dependencies library. I sure it.

please look that I define in tests.cpp not in header file or in build system.

all assert in somewhere else disable, because I change NDEBUG locally in tests.cpp and after including sqlite_orm.h, and back to original value after including cassert.

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

I see but once we need a generic assert in tests and game over cause assert must fire on debug build not in release build. But our tests will have the same behavior for both cases

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

Let me change it quickly in a 5 min

@soroshsabz
Copy link
Contributor Author

Let me change it quickly in a 5 min

Ok, any way you like :)

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

Merged in #262 . Sorry for being late

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 2, 2019

@fnc12 no problem, thanks for working, but this PR #262, does not resolve above problem.

the problem in release mode is assert does not call so for example below code does not call

 assert(user3.id == storage.insert(user3, columns(&User::id, &User::name, &User::age, &User::email)));

and then

auto insertedUser3 = storage.get<User>(user3.id);

does not work, because insert does not call before.

The another note is in release mode all check in tests.cpp ( all assert in all functions) does not work and practically you do not test any thing if you do not define assert in release mode.

I think the fast and good solution to resolve this problem is to undef NDEBUG locally.

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

Sh*t. I was in a hurry. Sorry. I'll fix it in 10 min

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 2, 2019

@fnc12 Ok, no problem, but still I think assert is good solution :D

We can use assert until switching to GTest :)

thanks very much for follow up this PR :)

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

fixed in #263

@soroshsabz
Copy link
Contributor Author

soroshsabz commented Apr 2, 2019

@fnc12 Thanks for replaying, but it is not work too, above line is just example, for example another line is

            assert(visit2.id() == storage.insert(visit2, columns(&Visit::setId, &Visit::setUsedId)));

I think when disable assert, most of tests is meaningless , why you want to do this? ( you can not check behavior is correct, because assert does not working. and you must change all assert in tests.cpp if you want to check behavior is correct )

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

OMG. I still sure that defining/undefining NDEBUG is a bad idea. If we decide to run unit tests with release configuration them we must change assert to something that runs well in release (gtest's VERIFY as you said before). So for this PR it would be quicker to fix asserts` arguments cause it splits tasks. I'll fix it in 15 mins

@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

#265 fixed here. Now it must work anyway

@soroshsabz soroshsabz force-pushed the feature/258-add-appveyor-config branch from 7aa9301 to 80ddd16 Compare April 2, 2019 16:31
@soroshsabz
Copy link
Contributor Author

@fnc12 Ok, thanks to @fnc12 all test now pass without undef NDEBUG, but now all assert does not check behavior of sqlite_orm functionalities. so you agree with this :)

now I think this PR can be merge if you agree :)

thanks very much

@fnc12 fnc12 merged commit 914b542 into fnc12:dev Apr 2, 2019
@fnc12
Copy link
Owner

fnc12 commented Apr 2, 2019

thanks to you

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.

2 participants