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.

Sign up for GitHub

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

Jump to bottom

Fix windows tests #1560

Closed
wants to merge 3 commits into from
Closed

Fix windows tests #1560

wants to merge 3 commits into from

Conversation

talregev
Copy link
Contributor

Fix windows tests:

  1. compile with ninja
  2. 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. from Fix Windows Tests #1553
  3. GTSAM_EXPORT on functions.
  4. GTSAM_EXPORT on nested structs / classes.

@talregev
Copy link
Contributor Author

talregev commented Jun 30, 2023 •
edited
Loading

I still haven't fix all the problems, but I succeeded to eliminate the missing symbol that was easy for me to fix.
And now I need your help to deal with the duplicate symbol.
@varunagrawal Can you review and help?

@talregev
Fix linkage error:
8ef4475 
- GTSAM_EXPORT on functions.
- GTSAM_EXPORT on nested struts / classes.
@varunagrawal
Copy link
Collaborator

Hi @talregev. Can you please avoid force-pushing? That will make reviewing this PR easier for us and help us keep track of changes. Thanks again for all the help.

@talregev
Copy link
Contributor Author

I am done edit this PR. feel free to review. Thank you!

varunagrawal
varunagrawal reviewed Jun 30, 2023
View reviewed changes
.github/workflows/build-windows.yml
@@ -16,19 +20,15 @@ jobs:
BOOST_EXE: boost_1_72_0-msvc-14.2

strategy:
fail-fast: true
fail-fast: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please explain the rationale behind changing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Because I don't want to fail Release or Debug if the other one is fail. Because they can fail from other reasons. So it better let them to fail by their own.

gtsam/geometry/Pose2.h
struct ChartAtOrigin {
GTSAM_EXPORT static Pose2 Retract(const Vector3& v, ChartJacobian H = {});
GTSAM_EXPORT static Vector3 Local(const Pose2& r, ChartJacobian H = {});
struct GTSAM_EXPORT ChartAtOrigin {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is incorrect since ChartAtOrigin is an inner class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ChartAtOrigin is inner class. As I discovered from the errors. It also require an export. Other wise it missing their functions symbols. The GTSAM_EXPORT on Pose2 class it not enough.

gtsam/geometry/Pose3.h
@@ -204,7 +204,7 @@ class GTSAM_EXPORT Pose3: public LieGroup<Pose3, 6> {
static Matrix6 LogmapDerivative(const Pose3& xi);

// Chart at origin, depends on compile-time flag GTSAM_POSE3_EXPMAP
struct ChartAtOrigin {
struct GTSAM_EXPORT ChartAtOrigin {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. ChartAtOrigin is an inner class/struct.

gtsam/geometry/Rot2.h
@@ -141,7 +141,7 @@ namespace gtsam {
}

// Chart at origin simply uses exponential map and its inverse
struct ChartAtOrigin {
struct GTSAM_EXPORT ChartAtOrigin {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

gtsam/geometry/Rot3.h
@@ -396,7 +396,7 @@ class GTSAM_EXPORT Rot3 : public LieGroup<Rot3, 3> {
Matrix3 AdjointMap() const { return matrix(); }

// Chart at origin, depends on compile-time flag ROT3_DEFAULT_COORDINATES_MODE
struct ChartAtOrigin {
struct GTSAM_EXPORT ChartAtOrigin {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

gtsam/geometry/Similarity2.h
@@ -150,7 +150,7 @@ class GTSAM_EXPORT Similarity2 : public LieGroup<Similarity2, 4> {
OptionalJacobian<4, 4> Hm = {});

/// Chart at the origin
struct ChartAtOrigin {
struct GTSAM_EXPORT ChartAtOrigin {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

.github/workflows/build-windows.yml
@@ -87,10 +87,15 @@ jobs:
- name: Checkout
uses: actions/checkout@v3

- name: Setup msbuild
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think some of these changes are the reason why the CI isn't running on this PR?

Copy link
Contributor Author

@talregev talregev Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The CI is not running because I am new to this repo. (I didn't commit any changes to this repo). And you need activity approve the ci after each change I made. (Very annoying for me and for you).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any CIs in the Checks tab of this PR. Are you sure it's something related to admin privileges?

Copy link
Contributor Author

@talregev talregev Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

image

The review require, mean some one need to approve my CI. It already done here
#1413 with @dellaert.

Soon I will close this PR, and open a new one better results.
But this approve also needed for other PR to show my CI.
Do you have the privilege to approve it?

I already successfully run the ci on my repo. That how I manage to develop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have the privilege, since I approved the CI for #1559. But I don't see the button appearing for this one...

Copy link
Contributor Author

@talregev talregev Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

By closing and opening you approve it indirectly.
But I don't have the privilege to show you, which button to push. I just now it somewhere in the red section in the printscreen. You should approve the review, and not the PR.
I want to close this PR, and open a new one.

@varunagrawal varunagrawal reopened this Jun 30, 2023
@talregev
Copy link
Contributor Author

I am open a new better PR to handle the error in windows tests.

@talregev talregev closed this Jun 30, 2023
@varunagrawal
Copy link
Collaborator

Wait why did you close the PR?

@talregev
Copy link
Contributor Author

Wait why did you close the PR?

Because it generate 80 errors of multiple definition of symbols, and I already have a better PR.

@varunagrawal
Copy link
Collaborator

Okay, then why not just push to this PR? I'm having some difficulty keeping track of the different PRs...

@talregev
Copy link
Contributor Author

Okay, then why not just push to this PR? I'm having some difficulty keeping track of the different PRs...

Because you against force push, and I already open a new branch.

@varunagrawal
Copy link
Collaborator

You can force push in special circumstances like this one. Please be sure to run git merge <branch> and not git rebase <branch>.

@talregev talregev deleted the TalR/fix/windows-tests branch June 30, 2023 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varunagrawal varunagrawal Awaiting requested review from varunagrawal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@talregev @varunagrawal