strands-project / strands_executive

Executive control code for STRANDS robots.
10 stars 19 forks source link

Prism embed #302

Closed marc-hanheide closed 7 years ago

marc-hanheide commented 7 years ago

this embed prism as an external project. Probably still needs some fixes by @bfalacerda to start it properly, but it downloads the latest prism, and then can also install and release it. prism can then be started with rosrun prism_strands prism <ARGS>.

marc-hanheide commented 7 years ago

retest this please

marc-hanheide commented 7 years ago

OK, routine_behaviours is not released yet... https://lcas.lincoln.ac.uk/buildfarm/job/Kpr__strands_executive__ubuntu_xenial_amd64/1/

marc-hanheide commented 7 years ago

I lost track, @bfalacerda what is the release status of strands_executive now?

bfalacerda commented 7 years ago

i think once #303 is merged it's ready. it builds on this PR.

marc-hanheide commented 7 years ago

OK, #303 is merged, let's see if this succeeds fine, now

marc-hanheide commented 7 years ago

Is routine_behaviours really needed for this? The problem is that this introduces circular dependencies, AFAICS. This is needed in the the strands_executive_tutorial/package.xml, so the problem is that the tutorial requires another package to be build, which in turn requires this to be released. It might actually already be enough to remove the <build_depend> line https://github.com/strands-project/strands_executive/blob/prism_embed/strands_executive_tutorial/package.xml#L26 ...

bfalacerda commented 7 years ago

@hawesie can you weigh in on this, I never touched the tutorial package

hawesie commented 7 years ago

Sorry, my plan was to remove it, but I didn't quite get around to it. It's on the list now.

hawesie commented 7 years ago

i.e. move strands_executive_tutorial to its own repo, probably with planning_tutorial which already exists.

hawesie commented 7 years ago

Looks like there is now an error compiling prism @bfalacerda

bfalacerda commented 7 years ago

What's the java version being used to compile prism in jenkins @marc-hanheide ?

hawesie commented 7 years ago

I've compared the compile on the build farm with a successful compile in the kinetic STRANDS docker image. The build farm is using java-7-openjdk-amd64 and gcc 4.8.4. The docker image is using java-8-openjdk-amd64 and 5.4.0.

hawesie commented 7 years ago

Ok, that's it. We need Java 8. First error:

jdd/../../src/prism/Model.java:69: error: illegal start of type
    default boolean hasLabelDD(String label)

default was added a method signature keyword in Java 8. I've reproduced this error compiling on 14.04 with Java 7.

hawesie commented 7 years ago

The problem here is that the rosdep key for Java is "default" which is 7 on 14.04 and 8 on 16.04. I will look at how to add java8 to the strands rosdep and see if we can use that to fix the problem.

hawesie commented 7 years ago

Alternatively (in case that isn't easy) is there a way to use an earlier version of prism which doesn't require Java 8 @bfalacerda?

hawesie commented 7 years ago

I could build successfully after installing openjdk-8-jdk in 14.04 using the below method. Unfortunately it's not clear how to automate this to

add-apt-repository ppa:webupd8team/java
apt-get update
apt-get install openjdk-8-jdk
hawesie commented 7 years ago

retest this please

bfalacerda commented 7 years ago

i guess we could use an earlier version of prism that compiles using java7 for indigo-devel and 14.04 releases. But for 16.04 it makes sense to keep the updated version.

marc-hanheide commented 7 years ago

packaging up java8 for Indigo is not a good idea in my view. I think using an older prism version would have fewer impacts on other people. It is, however, possible to import that ppa into our repo, if you really want that.

hawesie commented 7 years ago

@bfalacerda it's your call.

hawesie commented 7 years ago

I think the problem with using an older prism version is that we have made STRANDS changes to this version in addition to the changes made in the core prism system, so going back is hard.

marc-hanheide commented 7 years ago

I am confused... the PPA contains only the following packages: image Enabling that PPA won't provide openjdk-8-jdk for trusty. I'm looking for the package elsewhere.

marc-hanheide commented 7 years ago

OK, I imported openjdk-8 from http://ppa.launchpad.net/openjdk-r/ppa/ubuntu/pool/main/o/openjdk-8/ and put it in our repos. It can now be installed via apt-get install openjdk-8-jdk for trusty amd64 from the normal STRANDS repos. I have also added a rosdep:

openjdk-8:
  ubuntu: [openjdk-8-jdk]

So, you can now depend the prism package on the key openjdk-8 and it should work.

Back to you @hawesie and @bfalacerda.

hawesie commented 7 years ago

Thanks, we're off again!

marc-hanheide commented 7 years ago

the error was expected, as the new buildfarm doesn't have the new rosdep in yet.

marc-hanheide commented 7 years ago

the default test is what we are looking at for now.

marc-hanheide commented 7 years ago

Even if it now installs jdk8, it still uses jdk7 to compile at the moment: https://lcas.lincoln.ac.uk/jenkins/job/pr-indigo-strands_executive/170/console

[  8%] Performing build step for 'prism'
VERSION: 4.3.1.dev
OSTYPE/ARCH: linux amd64
JAVA_DIR: /usr/lib/jvm/java-7-openjdk-amd64
JAVA_DIR_BACKUP: 
JAVAC: /usr/bin/javac
hawesie commented 7 years ago

Poop. With the above ppa it uses oracle-java8-set-default to configure java correctly. Is there an equivalent for the one you pulled? Or can @bfalacerda @davexparker patch prism-robots to prefer java 8 if around (or only compile with it, since it is required)

marc-hanheide commented 7 years ago

Well, the one I have included is openjdk, not oracle jdk. As above you provide a PPA that included the Oracle one, but then you stated to install openjdk-8-jdk which is NOT the one provided by the repo. We better stick to the openjdk one, as the Oracle one has legal implications... So, just get prism to use jdk8, I'd say. Not sure, how, though.

hawesie commented 7 years ago

Sorry, I copied older instructions than I used. I agree that fixing prism to pick up java 8 (for both build and runtime) is best.

hawesie commented 7 years ago

It looks like it's now building ok. The problem is that the tests are failing since it's trying to run with java 7 but the compiled classes use java 8.

hawesie commented 7 years ago

Could we merge it anyway and see what a release looks like? Or do the tests need to complete for a release to be created?

marc-hanheide commented 7 years ago

no, the tests don't need to be done for a release, BUT if the user hasn't got java8 as their default it would also fail on their end, wouldn't it?

hawesie commented 7 years ago

Yes. I guess we'll need to add some warning somewhere in the executable. @bfalacerda is that something you can add?

bfalacerda commented 7 years ago

i don't get exactly where that warning would be