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

Broken on Trusty (because of ASDF 3) #17

Closed
gaya- opened this issue Jun 25, 2014 · 11 comments
Closed

Broken on Trusty (because of ASDF 3) #17

gaya- opened this issue Jun 25, 2014 · 11 comments

Comments

@gaya-
Copy link
Member

gaya- commented Jun 25, 2014

So, I just tested roslisp on Trusty.
First of all, ros-load-manifest doesn't compile anymore.
Reason: it uses asdf:split-string, which is not external in ASDF 3 anymore.

The compilation error looks like this:

caught ERROR:
;   READ error during COMPILE-FILE:
;   
;     The symbol "SPLIT-STRING" is not external in the ASDF/INTERFACE package.
;   
;       Line: 262, Column: 35, File-Position: 12108
;   
;       Stream: #<SB-SYS:FD-STREAM
;                 for "file /home/eris/workspace/catkin/src/roslisp/load-manifest/load-manifest.lisp"
;                 {10099240C3}>
; compilation aborted after 0:00:00.070

I took a quick look and found two occurrences of asdf:split-string in ros_load_manifest/load-manifest.lisp.

As far as I understood, not all the functionality of load-manifest.lisp is actually used right now and many things are there only for backwards compatibility with really old systems. So, I'm not even sure if that particular piece of code is actually needed.

Unfortunately, I won't have time to look into this until Monday night.

@airballking, FYI.

@airballking
Copy link
Contributor

Thanks, @gaya- I'll look into it, tonight.

@airballking
Copy link
Contributor

I had a look at the code, but am not 100% sure (from reading) whether split-string is actually necessary functionality for us. Right now, I don't have roslisp_repl installed on my 14.04 system. I will experiment with this on my 12.04 system with printouts on Monday morning.

@airballking
Copy link
Contributor

I'm further digging into this. First observations:

  • load-manifest is indeed calling (asdf:initialize-source-registry ...)
  • asdf3 is split into two packages which are released together: asdf/defsystem (asdf itself) and uiop (some utilities)
  • split-string is part of uiop

@airballking
Copy link
Contributor

I attempted a first fix. You can find it here: airballking/roslisp@c980434

With this in place, load-manifest still works for me under Ubuntu 12.04 with ROS Hydro. @gaya- could you please check whether it also works under Ubuntu 14.04 with ROS Indigo? Thank you.

@gaya-
Copy link
Member Author

gaya- commented Jul 1, 2014

The fix looks good. I like your check for ASDF version. In some slime code I have seen a compile-time version of this checks (e.g. https://github.com/slime/slime/blob/master/contrib/swank-asdf.lisp#L56), which could be even more efficient.

On a more global scale, it looks like load-manifest is one big mess. The most clean (and most time consuming...) solution would be, IMO, to rewrite load-manifest (at least some functions of it) and the rest of roslisp that works with paths and code compilation / building in such a way that would have clean implementations for all the different versions of ASDF (maybe not ASDF 1, it is really old after all). Something like:

#+asdf1
(defun some-function ...)

#+asdf2
(defun some-function ...)

#+asdf3
(defun some-function ...)

The thing that bothers me the most is the way roslisp works with the ASDF source registry. It uses the interface from ASDF 1 (asdf:*central-registry*) which was reimplemented much nicer in the newer versions. I haven't looked into this that closely though, so I might be wrong...

In addition to that, if there really is much code that actually isn't used at all on modern platforms with modern ROS versions, it would be nice to either completely get rid of it, or deprecate it, or label it with #+asdf1 or something similar.

@moesenle, you seem to be the only person who has a comprehensive knowledge about what's happening in load-manifest. Could you, please, give us some input on this?

@bbrieber
Copy link

bbrieber commented Jul 2, 2014

I tested your fix and seems to work.
I was able to load cram packages in indigo.

I have not tested more complex stuff because I don't have my projects ported to indigo yet but it looks very promising...

@airballking
Copy link
Contributor

@bbrieber thanks for taking a look at this. If this fix makes roslisp usable with the new ASDF, I'd vote for sticking with the fix for the time being.

@gaya- I suggest we postpone cleaning up load-manifest until after the ICRA submission deadline in autumn. What do you think?

@gaya-
Copy link
Member Author

gaya- commented Jul 3, 2014

Agreed! Things turned out much simpler than I expected.
If I could just have another 24 hours to test it with more complex packages that include C++ code and a foreign function interface, that'd be great.

@airballking
Copy link
Contributor

@gaya- Sure! I think a couple of days are no problem at all.

@gaya-
Copy link
Member Author

gaya- commented Jul 6, 2014

Ok, I tested it on more complex projects, works well. I do get linkage errors from CFFI on really complex rosbuild packages but I'm pretty certain it has nothing to do with roslisp. Let's merge the fix.

@airballking
Copy link
Contributor

Done. @Gaya thanks for testing!

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

No branches or pull requests

3 participants