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

open62541: add scripts to generate custom servers #7314

Merged
merged 27 commits into from
Oct 20, 2021
Merged

open62541: add scripts to generate custom servers #7314

merged 27 commits into from
Oct 20, 2021

Conversation

Mo-Tay
Copy link
Contributor

@Mo-Tay Mo-Tay commented Sep 16, 2021

Building the project with the CMake Macro "ua_generate_nodeset_and_datatypes" provided in in "[https://opcua.rocks/step-9-using-a-custom-uanodeset-with-open62541/]" gave the following exception:

`CMake Error at /home/xx/.conan/data/open62541/1.2.2/sp/prerelease/package /c32e2434a9be6dcdf5509b2f4986160a64168842/lib/cmake/open62541/open62541Macros.cmake:456 (message):

open62541_TOOLS_DIR must point to the open62541 tools directory Call Stack (most recent call first): xx/xx/FooServer/CMakeLists.txt:10 (ua_generate_nodeset_and_datatypes)`

see issue #7312
Fix:

  • open62541 OPC UA library needs to be modified to include the files in tools directory needed to create an OPC UA using the nodeset files generated from UA-ModelCompiler
  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

Sign in to view
@madebr
Copy link
Contributor

madebr commented Sep 16, 2021 •
edited
Loading

I don't fully understand these changes.
You don't actually add a requirement on ua-nodeset.

Who does provide the cmake function ua_generate_nodeset_and_datatypes?

madebr
madebr reviewed Sep 16, 2021
View reviewed changes
recipes/open62541/all/conanfile.py Outdated
@@ -361,6 +377,12 @@ def package_info(self):
os.path.join("include", "plugin")
]

#required for creating servers from ua nodesets
self.env_info.open62541_TOOLS_DIR = os.path.join(self.package_folder, "share", "tools")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a cmake variable, not an environment variable.
https://github.com/open62541/open62541/search?q=open62541_TOOLS_DIR
I think you need to add this variable to a cmake script.
Like lib/cmake/open62541/open62541Macros.cmake, but then specific for this variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using an environment variable is problematic when using a 2 profile build (one for host and one for build machine), because environment variables from host requirements are not applied.

madebr
madebr reviewed Sep 16, 2021
View reviewed changes
recipes/open62541/all/conanfile.py Outdated
Comment on lines 353 to 356
self.copy("generate_*.py", dst="share/tools", src=self._tools_subfolder)
self.copy("nodeset_compiler/*", dst="share/tools", src=self._tools_subfolder)
#use local local ua-nodeset instead of the OPC Founation files (NOT RECOMMENDED) Use the UA-Nodeset package instead
#self.copy("ua-nodeset/*", dst="share", src=self._deps_subfolder)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.copy("generate_*.py", dst="share/tools", src=self._tools_subfolder)
self.copy("nodeset_compiler/*", dst="share/tools", src=self._tools_subfolder)
#use local local ua-nodeset instead of the OPC Founation files (NOT RECOMMENDED) Use the UA-Nodeset package instead
#self.copy("ua-nodeset/*", dst="share", src=self._deps_subfolder)
self.copy("generate_*.py", src=self._tools_subfolder, dst=os.path.join("res", "tools"))
self.copy("nodeset_compiler/*", src=self._tools_subfolder, dst=os.path.join("res", "tools"))

madebr
madebr reviewed Sep 16, 2021
View reviewed changes
recipes/open62541/all/conanfile.py Outdated
Comment on lines 331 to 337
return os.path.join(self._source_subfolder, "tools")

# @property
# def _deps_subfolder(self):
# return os.path.join(self._source_subfolder, "deps")

def package(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return os.path.join(self._source_subfolder, "tools")
# @property
# def _deps_subfolder(self):
# return os.path.join(self._source_subfolder, "deps")
def package(self):
return os.path.join(self._source_subfolder, "tools")
def package(self):

madebr
madebr reviewed Sep 16, 2021
View reviewed changes
recipes/open62541/all/conanfile.py Outdated
@@ -322,10 +322,18 @@ def _configure_cmake(self):

def build(self):
for patch in self.conan_data.get("patches", {}).get(self.version, []):
tools.patch(**patch)
tools.patch(**patch)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tools.patch(**patch)
tools.patch(**patch)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madebr I add the requirement for the ua-nodeset directly in my application "the OPC UA server" since probably I will only require it when I am creating a server using the ua-nodeset compiler.

regarding the ua_generate_nodeset_and_datatypes "to my knowledge" the macro was written by @Pro he has also made a nice tutorial for creating servers using nodeset files https://opcua.rocks/step-9-using-a-custom-uanodeset-with-open62541/#comment-893

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Mo-Tay
Please see Mo-Tay#1.
I "borrowed" @Pro's example in there. It would be great if it got reduced somehow.
As a open62541 test, we're only interested in checking whether the basic functionality works.

@madebr
Copy link
Contributor

madebr commented Sep 17, 2021

Please consider Mo-Tay#1
Also, please rename the topic of this pr to something like open62541: add scripts to generate custom servers

@Mo-Tay Mo-Tay changed the title feat: add tools files, variables for ua-nodeset open62541: add scripts to generate custom servers Sep 17, 2021
@conan-center-bot

This comment has been minimized.

Sign in to view
madebr and others added 2 commits September 20, 2021 15:55
@conan-center-bot

This comment has been minimized.

Sign in to view
@prince-chrismc prince-chrismc mentioned this pull request Sep 21, 2021
Open
@conan-center-bot

This comment has been minimized.

Sign in to view
@Pro
Copy link
Contributor

Pro commented Sep 21, 2021

This package depends on #7312

@Pro Pro mentioned this pull request Sep 25, 2021
Merged
7 tasks
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@Mo-Tay Mo-Tay force-pushed the feat-open62541-ua-nodeset branch from 07a29f7 to 0ee8f8f Compare October 9, 2021 23:18
@conan-center-bot

This comment has been minimized.

Sign in to view
@Mo-Tay Mo-Tay force-pushed the feat-open62541-ua-nodeset branch from 0ee8f8f to db69aae Compare October 10, 2021 00:00
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@Mo-Tay Mo-Tay force-pushed the feat-open62541-ua-nodeset branch from 0aba13b to fa42f53 Compare October 11, 2021 14:08
@conan-center-bot
Copy link
Collaborator

All green in build 35 (fa42f5356c999e98e396dc1cbd1d3260372a0291):

  • open62541/1.0.3@:
    All packages built successfully! (All logs)

  • open62541/1.0.6@:
    All packages built successfully! (All logs)

  • open62541/1.1.3@:
    All packages built successfully! (All logs)

  • open62541/1.1.5@:
    All packages built successfully! (All logs)

  • open62541/1.1.6@:
    All packages built successfully! (All logs)

  • open62541/1.2.2@:
    All packages built successfully! (All logs)

@prince-chrismc
Copy link
Contributor

This package depends on #7312

This new recipe is not a requirement 🤔 what is the depency?

@Pro
Copy link
Contributor

Pro commented Oct 12, 2021

This package depends on #7312

This new recipe is not a requirement 🤔 what is the depency?

The open62541 package (i.e., the library) is not directly dependent on ua-nodest. I.e., ua-nodeset is not required to build the library.

The ua-nodeset is required when you build an executable using the library and creating an OPC UA server.
Therefore, the ua-nodeset is only required in the test_package, which is using the nodeset compiler added via this MR.

@prince-chrismc
Copy link
Contributor

Thank you for explaining!

@conan-center-bot conan-center-bot merged commit 0e332d8 into conan-io:master Oct 20, 2021
ivanvurbanov pushed a commit to ivanvurbanov/conan-center-index that referenced this pull request Dec 2, 2021
@Mo-Tay @madebr
@Pro @ivanvurbanov
(conan-io#7314) open62541: add scripts to generate custom servers
c6074d2 
* feat: add tools files, variables for  ua-nodeset

* open62541: add custom server example

* open62541: rename test package exe + fix path of exe in chmod +x

* fix: rename nodeset_path to nodeset_dir

* fix: use the correct ua-nodeset version

* fix: add NAMESPACE_IDX for older versions

* fix: minimize custom server for testing

* fix: add newlines

* fix: update modify model csv file

* fix: add custom DataType for nodeset test server

* fix: add newline

* fix: decrease server runtime for test packages

* fix: triger build

* fix: use different server's port, decrease runtime

* fix: use NULL instead of nullptr

* fix: disable Windows shared build for 1.2.2

* fix: use C++ for test package

* fix: use c++ for test package

* Revert "fix: use c++ for test package"

This reverts commit 39dcb20.

* Revert "fix: use C++ for test package"

This reverts commit 0b902c0.

* fix: remove server default confing

* fix: start test server on different port

* fix: remove repeated test, corret port adaptation

* fix: older versions issues

* feat: formating

Co-authored-by: Anonymous Maarten <anonymous.maarten@gmail.com>
Co-authored-by: Stefan Profanter <stefan.profanter@agile-robots.com>
@valgur valgur mentioned this pull request Jun 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madebr madebr madebr left review comments

@prince-chrismc prince-chrismc prince-chrismc approved these changes

@danimtb danimtb danimtb approved these changes

@SSE4 SSE4 SSE4 approved these changes

@uilianries uilianries Awaiting requested review from uilianries

@czoido czoido Awaiting requested review from czoido

@jgsogo jgsogo Awaiting requested review from jgsogo

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

Successfully merging this pull request may close these issues.
8 participants
@Mo-Tay @CLAassistant @conan-center-bot @madebr @Pro @prince-chrismc @SSE4 @danimtb