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

FMU-proxy orchestration support #239

Merged
merged 56 commits into from
Jun 5, 2019

Conversation

markaren
Copy link
Contributor

@markaren markaren commented May 7, 2019

FMU-proxy support for PR #233

@markaren markaren mentioned this pull request May 7, 2019
@markaren markaren changed the title Implement cse::model for remote_fmu FMU-proxy orchestration support May 7, 2019
@markaren markaren requested review from kyllingstad, hplatou and ljamt May 8, 2019 08:32
@markaren markaren self-assigned this May 8, 2019
kyllingstad and others added 9 commits May 9, 2019 08:43
We have no need for model_directory and slave_uri_resolver at the moment.
It is better to re-introduce them at a later time if needed.
…urely, add support for relative file paths for fmuproxy
…eric-fmu-interface-fmuproxy

# Conflicts:
#	include/cse/orchestration.hpp
…e' into feature/165-generic-fmu-interface-fmuproxy

# Conflicts:
#	src/cpp/ssp_parser.cpp
@markaren
Copy link
Contributor Author

I've updated the PR to use the new iteration. I added some ad-hoc changes to the original code. Should be fixed upstream and merged back.
Cavecat with fmu-proxy file support. Files MUST be relative.

@markaren
Copy link
Contributor Author

markaren commented Jun 3, 2019

I guess we could modify orchestration.cpp with something like:

#if __has_include(<cse/fmuproxy/fmuproxy_uri_sub_resolver.hpp>)
#DEFINE HAS_FMUPROXY
#include <cse/fmuproxy/fmuproxy_uri_sub_resolver.hpp>
#endif

....

std::shared_ptr<model_uri_resolver> default_model_uri_resolver()
{
    auto resolver = std::make_shared<model_uri_resolver>();
    resolver->add_sub_resolver(std::make_shared<file_uri_sub_resolver>());
#ifdef HAS_FMUPROXY
  resolver->add_sub_resolver(std::make_shared<fmuproxy_uri_sub_resolver>());
#endif
    return resolver;
}

?

@ljamt
Copy link
Member

ljamt commented Jun 3, 2019

We concluded to keep the fmu-proxy flag last meeting, so we need something like that yes. We can discuss this tomorrow.

@markaren
Copy link
Contributor Author

markaren commented Jun 3, 2019

I'm travelling btw, but I'll be available at slack if there should be any discussion afterwards.

@ljamt ljamt requested a review from kyllingstad June 4, 2019 08:44
@ljamt
Copy link
Member

ljamt commented Jun 4, 2019

We are getting close to merge this PR. How shall we do that? This PR is requesting to merge into feature/165-generic-fmu-interface

BTW, I think your proposal for adding the sub_resolver is good @markaren

@markaren
Copy link
Contributor Author

markaren commented Jun 4, 2019

I would suggest to merge #233 first, then change the target of this to master.

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

Looks good now!

We'll need to figure out what to do about running the tests, but that was a pre-existing problem that is not related to this PR.

@kyllingstad
Copy link
Member

I would suggest to merge #233 first, then change the target of this to master.

I agree, this is the cleanest way to do it.

@ljamt
Copy link
Member

ljamt commented Jun 4, 2019

Linux build agent is up and running again, and Build on Linux with Conan & FMU-proxy support build currently fails.

@kyllingstad
Copy link
Member

BTW, I think your proposal for adding the sub_resolver is good @markaren

Same here. It could be added in the final PR (to master).

@markaren
Copy link
Contributor Author

markaren commented Jun 4, 2019

The error is

/home/****/****_slave/workspace/5-generic-fmu-interface-fmuproxy/cse-core/src/cpp/fmuproxy/remote_fmu.cpp:7:0: error: ignoring #pragma warning  [-Werror=unknown-pragmas]

 #pragma warning(push)

 
/home/****/****_slave/workspace/5-generic-fmu-interface-fmuproxy/cse-core/src/cpp/fmuproxy/remote_fmu.cpp:8:0: error: ignoring #pragma warning  [-Werror=unknown-pragmas]

 #pragma warning(disable : 4245)


/home/****/****_slave/workspace/5-generic-fmu-interface-fmuproxy/cse-core/src/cpp/fmuproxy/remote_fmu.cpp:12:0: error: ignoring #pragma warning  [-Werror=unknown-pragmas]

 #pragma warning(pop)

How to best resolve @kyllingstad ?

Add a #ifdef __WIN32 perhaps ?

@markaren
Copy link
Contributor Author

markaren commented Jun 4, 2019

Thrift does:

#ifdef _MSC_VER
  #pragma warning( push )
  #pragma warning (disable : 4250 ) //inheriting methods via dominance 
#endif

which is better than using #ifdef _WIN32 I guess

@markaren
Copy link
Contributor Author

markaren commented Jun 4, 2019

There is another linux error. Will fix when I get back to a linux machine tomorrow.

@kyllingstad
Copy link
Member

which is better than using #ifdef _WIN32 I guess

Yes, #pragma warning is MSVC specific.

There is another linux error.

Only the #pragma directives should be enclosed in #ifdef _MSC_VER, not the #include directives.

@markaren markaren changed the base branch from feature/165-generic-fmu-interface to master June 4, 2019 11:03
@ljamt
Copy link
Member

ljamt commented Jun 4, 2019

Jenkins is happy 🎉

@ljamt
Copy link
Member

ljamt commented Jun 4, 2019

Then it's only the final modification of orchestration.cpp to include the sub resolver left.

@markaren
Copy link
Contributor Author

markaren commented Jun 5, 2019

#if __has_include(<cse/fmuproxy/fmuproxy_uri_sub_resolver.hpp>) seems to be true with FMUPROXY=OFF ..

@markaren
Copy link
Contributor Author

markaren commented Jun 5, 2019

This is caused by

target_include_directories(csecorecpp
    PUBLIC
        "$<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include>"
    PRIVATE
        "${CMAKE_CURRENT_SOURCE_DIR}"
)

no?

Should we rather specify a add_definition("WITH_FMUPROXY") in the CMake file? @kyllingstad

@kyllingstad kyllingstad self-requested a review June 5, 2019 10:09
Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

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

Great!

@markaren markaren merged commit 98e8d4b into master Jun 5, 2019
@markaren markaren deleted the feature/165-generic-fmu-interface-fmuproxy branch June 11, 2019 06:37
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.

3 participants