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

Fix windows tests #1562

Merged
merged 9 commits into from
Jul 1, 2023
Merged

Conversation

talregev
Copy link
Contributor

My commits above yours give better results:

  1. Add fix for compilation in Visual Studio. This seems to be a weird issue with how size_t is interpreted since the multiple symbols defined error comes from additional declarations of std::map<Key, size_t>. This might be a bug in MSVC 16.
  2. Fixed some more tests across the repo.
  3. Updated the Windows CI to run additional GTSAM tests as well as some gtsam_unstable tests.
  4. Compile with ninja
  5. GTSAM_EXPORT on functions.
  6. GTSAM_EXPORT on nested structs / classes.

@talregev
Copy link
Contributor Author

@varunagrawal If you can approve the ci it will be great.
You can also look on the CI on my repo:
talregev#8

@talregev
Copy link
Contributor Author

image

@varunagrawal varunagrawal changed the base branch from develop to fix/windows-tests June 30, 2023 18:59
@varunagrawal varunagrawal changed the base branch from fix/windows-tests to develop June 30, 2023 19:00
@varunagrawal
Copy link
Collaborator

Why not checkout my fix branch, make a branch off that and then target this PR to my branch? That will make the history clean and avoid duplication.

@talregev
Copy link
Contributor Author

talregev commented Jun 30, 2023

Why not checkout my fix branch, make a branch off that and then target this PR to my branch? That will make the history clean and avoid duplication.

Because it has conflict with develop when I do that. I solve also the conflict.

@varunagrawal
Copy link
Collaborator

Why not checkout my fix branch, make a branch off that and then target this PR to my branch? That will make the history clean and avoid duplication.

Because it has conflict with develop when I do that. I solve also the conflict.

Go ahead and do that. I'll resolve the conflict with develop on my branch when we merge this to #1553.

@talregev
Copy link
Contributor Author

I guess I have more problems.

@talregev talregev marked this pull request as draft June 30, 2023 19:11
@varunagrawal
Copy link
Collaborator

Okay wait I am resolving this here.

@varunagrawal varunagrawal marked this pull request as ready for review June 30, 2023 19:16
@varunagrawal varunagrawal changed the base branch from develop to fix/windows-tests June 30, 2023 19:22
@varunagrawal
Copy link
Collaborator

@talregev you should now be able to push to this PR without problems.

@talregev
Copy link
Contributor Author

talregev commented Jun 30, 2023

@varunagrawal
Thank you for your merge, I hope it will not effect the solution.

I want to add:

concurrency:
  group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
  cancel-in-progress: true

To all workflows.
This code mean that every time I push a commit to my PR, the other check will cancel immediately and the runner (vm machine) will available for the next push. (This include any other person that open a PR). There is a limit for 20 machine per repo.
This make more rapid checks for the PRs.

@varunagrawal
Copy link
Collaborator

I love that idea! Also add a comment in the workflow file explaining that how you did over here. It is very useful!

@talregev
Copy link
Contributor Author

@varunagrawal Please approve again my CI :)

@talregev
Copy link
Contributor Author

It will affect on my next push.

@talregev
Copy link
Contributor Author

talregev commented Jun 30, 2023

@varunagrawal Can you cancel the actions on previous commit? It will take forever until it start running on the new commit.
Because I added the new code, only on the new commits, the runner will cancel automatic.

@talregev
Copy link
Contributor Author

The selected ones:

image

@talregev
Copy link
Contributor Author

talregev commented Jul 1, 2023

@varunagrawal I am done working on this branch.
There is one or more multiply defined symbols found errors that I don't know to solve for now.
Can you merge this branch to your windows fix and start play there?
Once you solve these errors, I will continue to open the rest of the tests and solve the other issue of linkages.
I will create a new branch and a new PR from your fix/windows-tests branch after the merge.

Thank you for your help.

@varunagrawal varunagrawal merged commit b62aa65 into borglab:fix/windows-tests Jul 1, 2023
@talregev talregev deleted the fix/windows3 branch July 1, 2023 10:12
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