Skip to content

Conversation

@amininger
Copy link
Contributor

This adds build support for SVS to the cmake/conan build system.

It also removes the old viewer code + unused tests folder from svs.

@moschmdt
Copy link
Contributor

I think the requirements are missing in the conanfile.py to fetch the dependencies?

try {
socket.close();
} catch (std::exception& e) {
// ignore
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 it would be better to just print something here, in case we run into a strange issue.

asio::connect(socket, endpoints);
is_connected = true;
} catch (std::exception& e) {
// ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, we should print something. If this connection fails, how does it affect the user? Does CMake fail, or does the viewer fail to connect?

#include "sgnode.h"

/****
* Note: This is a new implementation of the drawer socket that uses the asio library
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you quickly describe what the drawer socket is, or just link to another file that uses the term? It's un-googleable because of overlap with physical tools.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is an SGEL thing? I dunno. The file should be described in a more standalone way, I think, rather than referring to an old thing that is going to be deleted after we accept the CMake build.

@garfieldnate
Copy link
Collaborator

It's such a relief to say goodbye to the old viewer code 😌

I left a couple of comments and have one additional request:
please update build.yml so that the CMake tasks build Soar with SVS (the build should be for the most complete Soar possible).

Thank you for all this work!

project(soar
VERSION 9.6.3
LANGUAGES CXX)
LANGUAGES CXX C)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is C used for? I thought it was used in the viewer but not in the rest of SVS, but the viewer is going away. Or is it in a dependency?

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.

4 participants