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

rostypegen within arbitrary modules (default: Main) for __precompile__ compatibility #47

Merged
merged 3 commits into from Aug 10, 2018

Conversation

schmrlng
Copy link
Contributor

I've included the relevant doc change below. This commit also includes testing support for ROS Melodic.

I'll make another PR shortly that incorporates this commit and provides compatibility for Julia 0.7. There have been a decent number of changes to Expr syntax between 0.6 and 0.7, so instead of adding a bunch of if VERSION < v"0.7.0" statements I'm thinking that we drop support for 0.6 on master (a course many other Julia developers seem to be doing with the 0.7 update). That is, if this PR seems acceptable, this can be the last RobotOS 0.6 release, and we'll make a feature-equivalent 0.7 release that supports only Julia 0.7+. What do you think?


Compatibility with Package Precompilation

As described above, by default rostypegen creates modules in Main -- however,
this behavior is incompatible with Julia package precompilation. If you are using
RobotOS in the context of a module, as opposed to a script, you may reduce
load-time latency (useful for real-life applications!) by generating the ROS type
modules inside your package module using an approach similar to the example below:

# MyROSPackage.jl
__precompile__()
module MyROSPackage

using RobotOS

@rosimport geometry_msgs.msg: Pose
rostypegen(current_module())
import .geometry_msgs.msg: Pose
# ...

end

In this case, we have provided rostypegen with a root module (MyROSPackage)
for type generation. The Julia type corresponding to geometry_msgs/Pose now
lives at MyROSPackage.geometry_msgs.msg.Pose; note the extra dot in
import .geometry_msgs.msg: Pose.

@coveralls
Copy link

coveralls commented Jul 11, 2018

Coverage Status

Coverage increased (+0.1%) to 90.678% when pulling 6620b79 on schmrlng:precompile into df32f3c on jdlangs:master.

@schmrlng
Copy link
Contributor Author

Actually, this implementation doesn't quite work yet due to loss of state in RobotOS._rospy_objects when using the precompiled module. I'll update this PR when I find a workaround.

@schmrlng
Copy link
Contributor Author

I think the solution is just to re-@rosimport in the package __init__ function -- this is unavoidable for anything containing PyObjects in any case (i.e., "Using PyCall from Julia Modules" here)

# MyROSPackage.jl
__precompile__()
module MyROSPackage

using RobotOS

@rosimport geometry_msgs.msg: Pose
rostypegen(current_module())
import .geometry_msgs.msg: Pose
# ...

function __init__()
    @rosimport geometry_msgs.msg: Pose
    # ...
end

end

This precompilation interaction seems like a kind of a pain to test (but probably possible with some .travis.yml hacking). I'm inclined to say this case is not worth testing, since its resolution seems pretty clear.

@schmrlng
Copy link
Contributor Author

Bump -- I think this PR (and #48, as long as appropriate releases are set on METADATA.jl) should be invisible to existing users not looking for this sort of functionality, but I've been using it without any problems for the past week to cut a few seconds off of startup times.

@jdlangs
Copy link
Owner

jdlangs commented Jul 29, 2018

Hey @schmrlng, sorry it took me so long to find some time to look at this. I really appreciate your work here.

I agree with your plan going forward here. I was a bit uncomfortable at first with having the generated modules not look and act the same way as top-level package modules in 0.7. But after looking into it more, it looks like that won't be an option at all anyway and I like the pattern you're following of generating them inside the module of the user's application.

Regarding the extra, @rosimport statement, I would think we could get rid of it by adding an __init__ function to the generated modules that loads the PyObjects. If so, we can add that and then wrap up 0.6 support. If you have time to work on it that would be great since I really don't have much free time these days. Otherwise, we can merge this and maybe continue for a little while with joint 0.6/0.7 support. Let me know what you think.

### Compatibility with Package Precompilation
As described above, by default `rostypegen` creates modules in `Main` -- however,
this behavior is incompatible with Julia package precompilation. If you are using
`RobotOS` in the context of a module, as opposed to a script, you may reduce
Copy link
Owner

Choose a reason for hiding this comment

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

Can you rephrase this to something "using RobotOS in your own module or package..."? The phrase "in the context of a module" may be confusing.

src/gentypes.jl Outdated
@@ -277,38 +277,38 @@ function _import_rospy_pkg(package::String)
end

#The function that creates and fills the generated top-level modules
function buildpackage(pkg::ROSPackage)
function buildpackage(pkg::ROSPackage, rosrootmod::Module=Main)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove the Main default for these internal functions underneath rostypegen

@schmrlng
Copy link
Contributor Author

Hi @jdlangs - I agree with all of your suggestions; I'll try to implement the relevant changes within the next few days.

@schmrlng
Copy link
Contributor Author

schmrlng commented Aug 4, 2018

I think this is ready to go now for 0.6 (see the changes from before at ea29bda). I'm adding the __init__ method in all cases since I think it can't hurt. That is, it might not be strictly necessary if rosrootmod is Main during interactive/scripting use or for non-precompiled use-cases (I'm not even sure there's a way to check in general if rosrootmod is precompiled or not); let me know if you feel strongly that the __init__ method should not be included in such cases and I'll look into it.

I'll push analogous updates to #48 shortly.

Copy link
Owner

@jdlangs jdlangs left a comment

Choose a reason for hiding this comment

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

Thanks for following up! I just have the one question below.

src/gentypes.jl Outdated
imports = Expr[Expr(:import, :RobotOS, :AbstractMsg)]
othermods = filter(d -> d != _name(mod), mod.deps)
append!(imports, [Expr(:using,:Main,Symbol(m),:msg) for m in othermods])
append!(imports, [Expr(:using,Symbol(rosrootmod),Symbol(m),:msg) for m in othermods])
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be better to use relative path syntax here? Could rosrootmod be something that using won't see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it looks like I ended up going with absolute path in #48 (the 0.7 PR), i.e.,

Expr(:using, Expr(:., fullname(rosrootmod)..., Symbol(m), :msg))

for which the 0.6 equivalent here would be

Expr(:using, fullname(rosrootmod)..., Symbol(m), :msg)

I feel like since rosrootmod is the actual module (i.e., not a String or something) the absolute path approach should be infallible (knock on wood). I'll make the appropriate change to this PR (and to the commit history contained in #48).

Copy link
Owner

Choose a reason for hiding this comment

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

Cool, that works.

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.

None yet

3 participants