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 MSVC Build Errors #533

Closed

Conversation

johnwason
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.47%. Comparing base (f141bdd) to head (604a513).
Report is 5 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
- Coverage   84.49%   84.47%   -0.02%     
==========================================
  Files         233      233              
  Lines       15864    15861       -3     
==========================================
- Hits        13404    13399       -5     
- Misses       2460     2462       +2     

see 3 files with indirect coverage changes

@johnwason
Copy link
Contributor Author

@Levi-Armstrong can you clean up the code quality warnings and merge?

The error on Windows is just caused by trajopt_ifopt being missing.

@johnwason johnwason force-pushed the dev/win_build_failure branch from 3963e67 to 2be67c3 Compare December 2, 2024 18:06
Comment on lines -152 to +151
getDotgraph(const std::map<boost::uuids::uuid, std::unique_ptr<TaskComposerNodeInfo>>& results_map = {}) const;
std::string getDotgraph(const std::map<boost::uuids::uuid, std::unique_ptr<TaskComposerNodeInfo>>& results_map) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not keep the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSVC didn't like the = {} default. My recommendation is create an overload if you want the option to omit that parameter.

Comment on lines +175 to +179
/**
* @brief dump the task to dot
* @brief Return additional subgraphs which should get appended if needed
*/
virtual std::string dump(std::ostream& os) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the overload to fix the default parameter MSVC didn't like.

@@ -33,12 +33,12 @@ TESSERACT_COMMON_IGNORE_WARNINGS_PUSH
TESSERACT_COMMON_IGNORE_WARNINGS_POP

#include <tesseract_task_composer/core/task_composer_graph.h>
#include <tesseract_task_composer/core/task_composer_node_info.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I this necessary? Forward declaration should be good because it is only used in a return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSVC seems to have a slightly different method to handle std::unique_ptr that needs the full definition. Otherwise I get a "cannot delete incomplete type" build error.


/** @brief Generate the Dotgraph and save to file */
bool saveDotgraph(const std::string& filepath,
const std::map<boost::uuids::uuid, std::unique_ptr<TaskComposerNodeInfo>>& results_map = {}) const;
const std::map<boost::uuids::uuid, std::unique_ptr<TaskComposerNodeInfo>>& results_map) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MSVC doesn't like = {} defaults.

@johnwason
Copy link
Contributor Author

I need a version tag with these changes to finish the Python release.

@Levi-Armstrong
Copy link
Contributor

It looks like the windows CI is not passing; should it be?

@Levi-Armstrong
Copy link
Contributor

I will work on this over the weekend.

@johnwason
Copy link
Contributor Author

The failing test on Windows is caused by trajopt_ifopt not being built. tesseract-robotics/trajopt#431 will fix this but it is going to need more work.

@Levi-Armstrong
Copy link
Contributor

@johnwason I ported most of these change in #541 but need help tracking down the remaining build issues.

@Levi-Armstrong
Copy link
Contributor

Addressed by #542

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