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

HTTP API for CosmoScout #149

Merged
merged 19 commits into from
Apr 29, 2020
Merged

HTTP API for CosmoScout #149

merged 19 commits into from
Apr 29, 2020

Conversation

Schneegans
Copy link
Member

@Schneegans Schneegans commented Apr 28, 2020

I will add some documentation, but you can already start to play around! Simply checkout the branch, compile the externals and CosmoScout, launch it, and navigate to http://localhost:9001.

See cosmoscout/csp-web-api#1.

Edit: The plugin is not loaded per default now. You have to add this to you settings:

"csp-web-api": {
  "port": 9001,
  "page": "../share/resources/gui/example-web-frontend.html"
},

@Schneegans Schneegans added this to the Version 1.3.0 milestone Apr 28, 2020
@Schneegans Schneegans self-assigned this Apr 28, 2020
@github-actions
Copy link

Pull Request Test Coverage Report for Build 89782358

  • 0 of 8 (0.0%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 4.191%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/cs-utils/SafeQueue.hpp 0 8 0.0%
Files with Coverage Reduction New Missed Lines %
src/cs-utils/utils.hpp 4 55.56%
Totals Coverage Status
Change from base Build 89235985: -0.03%
Covered Lines: 570
Relevant Lines: 13600

💛 - Coveralls

@JonasGilg
Copy link
Member

On Windows I was promptet for admin privileges after (or at the end) of plugin loading. I denied the request and the applications seems to work normally.

I can't reproduce it, since Windows seems to remember the decision. (I will try and work around that).


cmake -E make_directory "%BUILD_DIR%/civetweb" && cd "%BUILD_DIR%/civetweb"
cmake %CMAKE_FLAGS% -DCMAKE_INSTALL_PREFIX="%INSTALL_DIR%" -DCIVETWEB_ENABLE_CXX=On ^
-DBUILD_SHARED_LIBS=On -DSPDLOG_ENABLE_PCH=On "%EXTERNALS_DIR%/civetweb" || exit /b
Copy link
Member

Choose a reason for hiding this comment

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

Is this right?

@JonasGilg
Copy link
Member

This plugin introduces a new external dependency that has to be installed for the whole of CosmoScout for a feature that is really specialized and only used within this plugin.

Do you think we can add dependencies of plugins that are only required by that plugin differently?
The first thing coming to my mind is using CMake's exernal projects feature: https://cmake.org/cmake/help/latest/module/ExternalProject.html

@Schneegans
Copy link
Member Author

This is a difficult question. With dependencies such as civetweb which can be build easily and do not drag in other things, I am quite happy to treat it like it's done now. This is especially true once we put the core plugins into the main repository. And it can happen that we use the library in other plugins in the future - curl, curlpp and cares were only required by the csp-lod-bodies plugin when we introduced it and now they are required by multiple plugins and cs-utils.

However there are more complicated dependencies (like VTK in the scivis branch). The best solution would be along these lines:

  • Get rid of make_externals.sh and make_externals.bat
  • In the main CMakeLists.txt we would call ExternalProject_Add for each core dependency.
  • Each plugin would call ExternalProject_Add for all additional dependencies. This however can lead to multiple calls to ExternalProject_Add for the same dependency. This would need to be resolved, especially if multiple plugins request different versions of the same dependency.
  • It also has to work for external dependencies which do not use CMake

What do you think? Would this be possible? But it's definitely not in the scope of this pull request...

@JonasGilg
Copy link
Member

Moved this discussion to an issue: #150

@Schneegans Schneegans marked this pull request as ready for review April 29, 2020 03:58
@Schneegans
Copy link
Member Author

I think I will remove the plugin from the default config, as it's not required for standard use cases. Are there any other ideas?

@Schneegans Schneegans merged commit 0ab360d into develop Apr 29, 2020
@Schneegans Schneegans deleted the feature/rest-api branch April 29, 2020 19:15
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