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

Add vcpkg ci #1413

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add vcpkg ci #1413

wants to merge 1 commit into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Jan 24, 2023

Add vcpkg ci. Fix #672

I try to minimize the changes as much as possible.
I delete the FindTBB.cmake. I think it old file, and the default of the cmake is working well also on your tests.
I also add to your workflow, that it will do test only when doing PR to devel and push to devel, as it your most use cases.
Also I add concurrency section, that it cancel automatic other check when you push to your PR another commit before the other check is not finish. It not doing that when you merge or push to devel, because each change there is important.
It useful to save resource and time for check your PRs.

Also there some bugs that I encounter that I think needed to fix in other PRs.
This PR already have many things to discuss.

This is start of infrastructure how to use vcpkg and can also extended (in other PRs). For example to publish binaries in the PR itself, that it can downloaded and test them externally.
Maybe other use cases you want to test.
My moto is to compile as much as possible in one test (and hopefully to run the tests themself), to know much quicker to the problem when you doing a PR.

I am not sure why the 2 of your workflow is failing, If it from stuff I did, I will happy to know, and try to fix accordingly.

gtsam bugs:
1. windows gtsam_unstable Solve!

gtsam.lib(gtsam.dll) : error LNK2005: "public: __cdecl std::map<unsigned __int64,unsigned __int64,struct std::less<unsigned __int64>,class std::allocator<struct std::pair<unsigned __int64 const ,unsigned __int64> > >::~map<unsigned __int64,unsigned __int64,struct std::less<unsigned __int64>,class std::allocator<struct std::pair<unsigned __int64 const ,unsigned __int64> > >(void)" (??1?$map@_K_KU?$less@_K@std@@V?$allocator@U?$pair@$$CB_K_K@std@@@2@@std@@QEAA@XZ) already defined in LPInitSolver.obj [D:\a\gtsam\gtsam\build\gtsam_unstable\gtsam_unstable.vcxproj]
     Creating library D:/a/gtsam/gtsam/build/lib/Release/gtsam_unstable.lib and object D:/a/gtsam/gtsam/build/lib/Release/gtsam_unstable.exp

2. Errors on tests: Solve!

/home/runner/work/gtsam/gtsam/gtsam/hybrid/tests/testHybridEstimation.cpp:221:8: error: ‘bitset’ is not a member of ‘std’
  221 |   std::bitset<K - 1> seq = x;
/home/runner/work/gtsam/gtsam/gtsam/hybrid/tests/testHybridEstimation.cpp:221:22: error: ‘seq’ was not declared in this scope
  221 |   std::bitset<K - 1> seq = x;
      |                      ^~~

3. windows python

LINK : fatal error LNK1149: output filename matches input filename 'D:\a\gtsam\gtsam\build\lib\Release\gtsam.lib' [D:\a\gtsam\gtsam\build\python\gtsam_py.vcxproj]

@dellaert
Copy link
Member

Cool, thanks for contributing !!! Workflow is failing for another reason, most likely OOM. I'm a bit swamped with getting boost out of 4.3 to land something like this right now, and I'll also like to learn a bit about this rather substantial PR. Can we have a call about this over the weekend or early next week?

@talregev
Copy link
Contributor Author

talregev commented Jan 24, 2023

Cool, thanks for contributing !!! Workflow is failing for another reason, most likely OOM. I'm a bit swamped with getting boost out of 4.3 to land something like this right now, and I'll also like to learn a bit about this rather substantial PR. Can we have a call about this over the weekend or early next week?

Yes, we can do on the weekend.
Can you approving review in my PR that it just run the tests?

@dellaert
Copy link
Member

Cool, thanks for contributing !!! Workflow is failing for another reason, most likely OOM. I'm a bit swamped with getting boost out of 4.3 to land something like this right now, and I'll also like to learn a bit about this rather substantial PR. Can we have a call about this over the weekend or early next week?

Yes, we can do on the weekend. Can you approving review in my PR that it just run the tests?

Will do! Send an email to [email protected] to set up a time :-)

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking a merge for now until I understand completely, but this looks cool!

@talregev
Copy link
Contributor Author

talregev commented Jan 24, 2023

Cool, thanks for contributing !!! Workflow is failing for another reason, most likely OOM. I'm a bit swamped with getting boost out of 4.3 to land something like this right now, and I'll also like to learn a bit about this rather substantial PR. Can we have a call about this over the weekend or early next week?

Yes, we can do on the weekend. Can you approving review in my PR that it just run the tests?

Will do! Send an email to [email protected] to set up a time :-)

Just type your comments here when you have time. If we need more interactive, I will send you an email :) Thanks!

@dellaert
Copy link
Member

Will do! Send an email to [email protected] to set up a time :-)

Just type your comments here when you have time. If we need more interactive, I will send you an email :) Thanks!

The email path is also so I can set up an invite.

@talregev
Copy link
Contributor Author

talregev commented Apr 30, 2023

@dellaert Are you still interested in these changes?

@talregev talregev force-pushed the TalR/vcpkg_ci branch 4 times, most recently from 27f55a6 to f8c0265 Compare April 30, 2023 12:48
@talregev
Copy link
Contributor Author

All the tests are passing:
talregev#1

@talregev talregev requested a review from dellaert May 1, 2023 20:27
@talregev talregev force-pushed the TalR/vcpkg_ci branch 3 times, most recently from a361a1b to 11d5363 Compare June 13, 2023 04:52
@talregev
Copy link
Contributor Author

talregev commented Jun 13, 2023

New things I develop from last time:

  • Rebase gtsam code to the latest dev branch.
  • All os compile with ninja compilation.
  • Tests compile and run on linux and macos.

Bugs:

  • On Windows it needed tbb debug on release compilation. I am not sure what this bug.
  • Tests don't compile on windows. missing symbols. Solve
  • Metis test is fail on run time.
  • Compile python on windows fail because out of memory. I think it not happen anymore.

@talregev
Copy link
Contributor Author

@varunagrawal
Can you take a look on my PR?

@varunagrawal
Copy link
Collaborator

varunagrawal commented Jun 18, 2023

@talregev I'm trying to fix #1541 before tackling this PR. Apologies for the delay.

@talregev
Copy link
Contributor Author

I also got this error. Not sure how to fix it. You can see the error above

@varunagrawal
Copy link
Collaborator

I also got this error. Not sure how to fix it. You can see the error above

Yup it's the same issue. Once we debug and fix that, I'll immediately review your PR. Thank you again for the amazing contribution and your patience.

@talregev talregev force-pushed the TalR/vcpkg_ci branch 7 times, most recently from 796401c to 7107cb3 Compare June 24, 2023 14:19
@talregev
Copy link
Contributor Author

@varunagrawal This PR is ready for review.
The tests fail because of their correctness. I don't have the knowledge to solve them. Can you help?
Other wise is ready for review and I will happy to hear your comments.

@talregev
Copy link
Contributor Author

talregev commented Dec 6, 2023

@dellaert This PR is ready for review.
Can you take a look?

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.

[Feature Request] Using Vcpkg/conan for dependencies and third party libraries
3 participants