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

Support msvc 2022 along 2019 #1715

Merged

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Jan 18, 2024

Fix #1642
Support msvc 2022 along 2019:

  • Change to toolset: 14.38
  • Fix ambiguous call to overloaded function CameraSet.h(331)

@varunagrawal
Copy link
Collaborator

I thought you had fixed the hanging CI issue?

@talregev
Copy link
Contributor Author

talregev commented Jan 23, 2024

I thought you had fixed the hanging CI issue?

I say probably will fix.
After my change The ci is much less to hang.
It was everytime, and now once for a while. So it improvement.
Also this PR is not about CI hanging.

@varunagrawal
Copy link
Collaborator

That's not quite what you said here: #1714 (comment)

Again, please don't make claims without evidence. I have less reason to trust your changes now.

@talregev
Copy link
Contributor Author

talregev commented Jan 23, 2024

As you can see from my first msg on the PR:
#1714 (comment)

Remove swap from python ci
It not needed for the ci.
Also may fix the hang problem.

Also you can see from other recent PRs here on gtsam that they are green and not red as they used to be.

You don't have to trust on my good changes. You can test them as long as you want.

And if this was bad change you already would reverted my change without talking to me, as you write on the PR.

@talregev
Copy link
Contributor Author

#1714 (comment)

On this comment I was talking about the swap that is not needed, because I show you how gcc not crash lacking the memory.
Hang problem happened on tests and not on compilation, It a known problem of the ci, and the side effect of the swap is amplify this problem.

@varunagrawal
Copy link
Collaborator

The other PRs are green because I've been re-running the CI until the test passes. I also have not reverted your changes, I don't understand why you're accusing me of that.

@talregev
Copy link
Contributor Author

talregev commented Jan 23, 2024

I don't accused you that you reverted my changes.
You would have if it was a bad change (Sorry about my bad grammar).
I just say It a good change, that you can run again the ci and make it green, because it happens once in awhile. Before the change it happens more. Almost every time.
And as I try to explain that the remove the swap was connected to gcc memory allocation when compiling. And the ci is hang on tests.
So you connect the memory issue to the ci hang, and all I am saying the swap ci amplify this problem.
Do you think that the is hang because of memory on the tests?
Also you say that you don't trust my changes and still benefits from them.
So I am not sure what are you trying to achieve by saying that, or keep discuss on non releted pr of that problem.
If you think my changes is not good (the remove of the swap)
You can revert the change (not recommended)
or Open a new issue about ci hang or remove swap and write me there.

And if you can please review this PR.

@talregev talregev force-pushed the TalR/support_multi_vcvars_versions branch from a6e32cc to ffb6268 Compare January 25, 2024 23:51
@talregev talregev force-pushed the TalR/support_multi_vcvars_versions branch from ffb6268 to 1a021ee Compare February 10, 2024 10:01
@talregev talregev force-pushed the TalR/support_multi_vcvars_versions branch 2 times, most recently from d42b51e to fa321bf Compare March 11, 2024 21:19
@talregev talregev force-pushed the TalR/support_multi_vcvars_versions branch from fa321bf to 5a81dc0 Compare March 11, 2024 21:34
@varunagrawal varunagrawal merged commit acf457e into borglab:develop Mar 18, 2024
31 checks passed
@talregev talregev deleted the TalR/support_multi_vcvars_versions branch March 22, 2024 10:56
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.

Building fails in VS22 due to ambiguous call
2 participants