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

First example of a Rust module implementation #344

Merged
merged 14 commits into from
Nov 3, 2023

Conversation

SirVer
Copy link
Contributor

@SirVer SirVer commented Sep 15, 2023

First Example of Rust Modules

This is modeled after the C++ Example modules, but shows up some more rustisms.

Co-authored-by: Dima Dorezyuk ddo@qwello.eu (@dorezyuk)
Co-authored-by: Kai Hermann (@hikinggrass)
Signed-off-by: Holger Rapp HolgerRapp@gmx.net

@SirVer SirVer force-pushed the hrapp/first_rust_module branch 2 times, most recently from 4870363 to 2155b88 Compare September 19, 2023 14:20
@SirVer SirVer force-pushed the hrapp/first_rust_module branch from 6b36828 to a55f0e3 Compare October 10, 2023 14:17
Comment on lines 561 to 562
COMMAND ${CMAKE_COMMAND} -E env EVEREST_RS_FRAMEWORK_SOURCE_LOCATION="${everest-framework_SOURCE_DIR}" EVEREST_RS_FRAMEWORK_BINARY_LOCATION="${everest-framework_BINARY_DIR}" ${CARGO} build
WORKING_DIRECTORY ${MODULE_BUILD_PATH}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I would use the generator expressions, to directly link to the correct library

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 do not understand what this means.

Copy link
Contributor

Choose a reason for hiding this comment

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

In cmake you have generator expressions, which can give you information about targets and other things. Say for example you have the library everest::framework (which resolves to a target), then you can get the full path to it via $<TARGET_FILE:everest::framework> instead of using guesses

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 defer here, not sure how to implement that.

@SirVer SirVer force-pushed the hrapp/first_rust_module branch from fd8af99 to 98fcc43 Compare October 24, 2023 09:40
@SirVer SirVer requested review from a-w50 and hikinggrass October 24, 2023 09:41
@SirVer
Copy link
Contributor Author

SirVer commented Oct 24, 2023

@hikinggrass I fixed the bug that the Rust modules did not properly initialize themselves, they were immediately dropped, hence no work was taking place. This should be working fine now, I think it is ready for merging if you are happy.

The sign-offs are messed up. Once we are all happy, I will rebase once and make sure this is all signed-off correctly by me, the co-authored are all mentioned in the PR description.

a-w50
a-w50 previously requested changes Oct 25, 2023
Copy link
Contributor

@a-w50 a-w50 left a comment

Choose a reason for hiding this comment

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

I wrote down some comments, I'll try to fix them

# workspace Cargo.toml, but we cannot easily pass it through the
# ev_add_module calls without polluting all CMakeLists.txt files
# with it. We bypass this using a global.
set_property(GLOBAL PROPERTY RUST_MODULE_LIST "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this one, but doesn't that reset the list all the time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Furthermore, unfortunately there is this global variable, but a bit cleaner might be to create a custom target with add_custom_target called something like everest_core_build_rust_modules, then, you can set a property on this target with set_property( target ...) (so we have at least some 'encapsulation')
furthermore, when running the add_rust_module ..., this function can add additional targets/dependencies to the everest_core_build_rust_modules if necessary, so we have proper dependency tracking

Copy link
Contributor Author

@SirVer SirVer Oct 25, 2023

Choose a reason for hiding this comment

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

I defer here, not sure how to implement that. As for resetting: it resets every time ev_add_project is called, so if this is called twice it would reset in between which is probably a problem. I am not sure how you are supposed to use the cMake stuff with out of repo builds, so I could not design for that.

return()
endif()

set(OUTPUT_FILE_PATH "${CMAKE_BINARY_DIR}/modules/Cargo.toml")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably choose not the the modules folder, rather the generated/rust-whatever..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Cargo.toml of the workspace must be in the top level of all Cargo modules. Having it in modules also allows to defer the exact location of where the binary ends up and makes sure that all rust modules use a consistent set of dependencies. I advise leaving it at its current place.


# Append each symlink path
foreach(SYMLINK ${RUST_MODULE_LIST})
file(APPEND "${OUTPUT_FILE_PATH}" " \"${SYMLINK}\",\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

cant we use just the full path here, instead of symlinking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, every member of a Cargo workspace must be in a subdirectory of a cargo workspace, hence symlinking is the only thing that works in the general case.

file(APPEND "${OUTPUT_FILE_PATH}" "]\n\n")

file(APPEND "${OUTPUT_FILE_PATH}" "[workspace.dependencies]\neverestrs = { path = \"")
file(APPEND "${OUTPUT_FILE_PATH}" "${everest-framework_SOURCE_DIR}/everestrs/everestrs")
Copy link
Contributor

Choose a reason for hiding this comment

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

Problem with ${everest-framework_SOURCE_DIR} is, that this only works with the integrated cpm build, not with yocto, it's also better here to use target properties

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 defer here, not sure how to implement that, especially since I have no idea how you build this stuff in Yocto. Would be interesting to see though, since we need to build our own Yocto layers too.

Comment on lines 564 to 565
execute_process(COMMAND rm -f "${MODULE_BUILD_PATH}")
execute_process(COMMAND ln -s "${MODULE_PATH}/" "${MODULE_BUILD_PATH}")
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use execute_process, first of all, this will always run during cmake configure stage, secondly it will not stop the cmake configure stage if it fails
we should use add_custom_command here, which we can tack onto the above mentioned target as a dependency

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 defer here, not sure how to implement that.

Comment on lines 573 to 577
find_program(
CARGO
cargo
REQUIRED
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not run for every add module, furthermore, try to things (that can fail) as early as possible, meaning, move it up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to where?

Comment on lines 588 to 593
add_custom_target(
${MODULE_NAME} ALL
COMMAND ${CMAKE_COMMAND} -E env EVEREST_RS_FRAMEWORK_SOURCE_LOCATION="${everest-framework_SOURCE_DIR}" EVEREST_RS_FRAMEWORK_BINARY_LOCATION="${everest-framework_BINARY_DIR}" ${CARGO} build ${CARGO_RELEASE_FLAG} --bin ${MODULE_NAME}
WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/modules
DEPENDS everest::framework
)
Copy link
Contributor

Choose a reason for hiding this comment

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

exactly, add this target as a dependency to the global everest_core_build_rust_modules target, and then do the symlinks inside here using commands

Comment on lines 600 to 601
PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE
DESTINATION "${EVEREST_MODULE_INSTALL_PREFIX}/${MODULE_NAME}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if the created binary already has X flag set, then it will also be installed like this

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, then we have to figure out why the created binary only has OWNER_READ and OWNER_WRITE set and no *_EXECUTE flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my system it is executable, but I have not tested what happens if the flags are removed.

Comment on lines +1 to +11
[workspace]
resolver = "2"
members = [
"RsExample",
"RsExampleUser",
]

[workspace.dependencies]
# This is not used when building with cmake, but you can change this for local
# development to point to a directory that works for you to build with cargo.
everestrs = { path = "../everest-framework/everestrs/everestrs" }
Copy link
Contributor

Choose a reason for hiding this comment

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

so this file would be managed by hand for the cargo only experience?

Copy link
Contributor

Choose a reason for hiding this comment

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

would it be an option, if we supply a sample file here (like Cargo.toml.sample), which then can be copied and modified by the rust developer to Cargo.toml, and putting it into .gitignore, so it won't be uploaded?

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 think that is the right solution. Are you doing this too when you brush up the cmake stuff, @a-w50 ?

@hikinggrass hikinggrass force-pushed the hrapp/first_rust_module branch from 7a82a15 to 6ddb0e0 Compare November 2, 2023 11:47
@SirVer
Copy link
Contributor Author

SirVer commented Nov 3, 2023

@hikinggrass What is this still waiting on?

@hikinggrass
Copy link
Contributor

@hikinggrass What is this still waiting on?

Had to track down a bug in the Cargo.toml generation for the rust_workspace, that's fixed now and I'm on my way to squashing it :)

@hikinggrass hikinggrass force-pushed the hrapp/first_rust_module branch from be8d100 to c4e7a78 Compare November 3, 2023 17:26
SirVer and others added 9 commits November 3, 2023 18:27
This is modelled after the C++ Example modules, but shows up some more rustisms.

Co-authored-by: Dima Dorezyuk <ddo@qwello.eu>
Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
And sleep in RsExampleUser.

Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
- in principle, the strategy is as before, just using some cmake idioms
- interesting question would be, if we can somehow setup the workspace
  via cmake so that rust only developers can use this directly with
  their IDE, we could make the RUST_WORKSPACE_DIR an user configurable
  option, and then, the user could use it directly

Signed-off-by: aw <aw@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
a-w50 and others added 5 commits November 3, 2023 18:27
- figure out how to find and parse this file

Signed-off-by: aw <aw@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: aw <aw@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: aw <aw@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass force-pushed the hrapp/first_rust_module branch from c4e7a78 to 383050b Compare November 3, 2023 17:27
@hikinggrass hikinggrass merged commit f9f7a8e into EVerest:main Nov 3, 2023
@SirVer
Copy link
Contributor Author

SirVer commented Nov 6, 2023

❤️ ❤️ 🎆 H O O R A Y 🚀 ❤️ ❤️

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