tarzanking / javacpp

Automatically exported from code.google.com/p/javacpp
GNU General Public License v2.0
0 stars 0 forks source link

Feature Suggestion : New annotation to specify output library name at java file. #29

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
This is a patch for new annotation @Library.
(Related discussion at 
https://groups.google.com/forum/#!msg/javacpp-project/Okyuh93c8Jc/sU4qrrHI4NsJ)

Add '@Library("MyLib")' at java class declaration then
- at build time, all class specified this annotation
will be generated into Mylib.cpp.
- at runtime, Loader.load() of this class will load libMyLib.so

If many java classes use same library, making static final String variable at 
somewhere and using it is good idea for maintainence.

* By definition, @Library can't be used with '-o' command option.
Even if '-o' option is used, @Library will be ignored at build time,
but at runtime Loader.load() still tries to load the annotated library. (you 
have to use Loader.loadLibrary("XXX") anyway.)

* Using @Library at inner class is not supported by current structure. It 
requires more modification.

Original issue reported on code.google.com by noranb...@gmail.com on 25 Nov 2012 at 2:11

Attachments:

GoogleCodeExporter commented 9 years ago
Looks good, thanks!

About nested classes, what kind of functionality are you expecting?

Original comment by samuel.a...@gmail.com on 1 Dec 2012 at 11:18

GoogleCodeExporter commented 9 years ago
Hum, nested classes or not, there's a problem we need to resolve before we may 
think about integrating your feature. Suppose we have two classes like this:

@Platform(include="bar1.h", link="bar1") @Library("foo") public class Bar1 { 
... }
@Platform(include="bar2.h", link="bar2") @Library("foo") public class Bar2 { 
... }

This code does not specify in which order the header files may be included or 
the library files linked. This isn't important in Java, but it's very very 
important in C/C++. Unless we can come up with a nice way to specify this 
order, I don't think it's a good idea to add something like your @Library 
annotation :(

Original comment by samuel.a...@gmail.com on 3 Dec 2012 at 2:55

GoogleCodeExporter commented 9 years ago
You've noticed a good point I haven't considered.
To solve this issue, I could think of some different approach, like annotated 
annotation @PlatformPreset.

For example,
Suppose javacpp provides
PlatformPreset.java
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.ANNOTATION_TYPE})
public @interface PlatformPreset {
    String value();
}

Then user could make all @Platform, @Library (or any generation annotation) to 
one java annotation. ex. MyPlatformPreset1.java, ... MyPlatformPresetN.java

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
@PlatformPreset(include={"bar1.h", "bar2.h"}, link={"lib1", "lib2"}, 
output="foo")
public @interface MyPlatformPreset1 {}

then your example could be rewritten like
Bar1.java
// same preset for Bar1 & Bar2 because they are generated into same cpp file.
@MyPlatformPreset1
public class Bar1 {...}

Bar2.java
// same preset for Bar1 % Bar2 because they are generated into same cpp file.
@MyPlatformPreset1
public class Bar2 {...}

Because @Platform, @Library is to generate one C library,
(Two java classes having same @Library but different @Platform make less sense.)
This @PlatformPreset approach seems more intuitive. (one preset to one C 
library)

Original comment by noranb...@gmail.com on 3 Dec 2012 at 4:52

GoogleCodeExporter commented 9 years ago
Maybe @PlatformPreset wouldn't be even necessary, if @Platform is extended like

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE, ElementType.METHOD, ElementType.ANNOTATION_TYPE}) // 
annotation_type is added.
public @interface Platform {
    String[] value()       default {};
    String[] not()         default {};
    String[] define()      default {};
    String[] include()     default {};
    String[] cinclude()    default {};
    String[] includepath() default {};
    String[] options()     default {};
    String[] linkpath()    default {};
    String[] link()        default {};
    String[] framework()   default {};
    String[] preloadpath() default {};
    String[] preload()     default {};
    String output()     default {}; // output of library name is added. it not specified, "jni" + javaClassName
}

Then MyPlatformPreset1.java will be

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
@Platform(include={"bar1.h", "bar2.h"}, link={"lib1", "lib2"}, output="foo")
public @interface MyPlatformPreset1 {}

Original comment by noranb...@gmail.com on 3 Dec 2012 at 5:09

GoogleCodeExporter commented 9 years ago
Yeah, I think that's a good idea! IMO "library" would be a better name for the 
annotation value than "output", because the latter sort of implies it's only 
going to be use on build, and because there are "library.prefix" and 
"library.suffix" properties already, which are related to this concept.

So, what about nested classes? If a user specifies

@Platform(...) @Namespace(...) // etc.
public class Foo {
    @Platform(...)
    public static class Bar { }
}

What happens to the annotation values of Foo within Bar?

Original comment by samuel.a...@gmail.com on 3 Dec 2012 at 9:41

GoogleCodeExporter commented 9 years ago
I think nested class should be treated independently.

If nested class has its own @Library(or @Platform), use it.
If nested class doesn't have @Library(or @Platform), use parent class' 
recursively.

This patch is still based on @Library, not @Platform(..., library=xxx)

Original comment by noranb...@gmail.com on 4 Dec 2012 at 2:28

Attachments:

GoogleCodeExporter commented 9 years ago
What about @Namespace and other things like that?

Original comment by samuel.a...@gmail.com on 5 Dec 2012 at 1:08

GoogleCodeExporter commented 9 years ago
When I tested I used @Namespace("::XXX") for inner class to not extend from
parent class.
But some users maybe want to extend namespace like @Namespace("XXX")

As long as @Namespace supports both extend and not-extend, it wouldn't be
an issue.

Original comment by noranb...@gmail.com on 13 Dec 2012 at 5:08

GoogleCodeExporter commented 9 years ago
So with something like this

@Platform(...) @Namespace("Foo")
public class Foo {
    @Platform(...)
    public static class Bar1 { }
    @Platform(...)
    public static class Bar2 { }
}

Both Bar1 and Bar2 would belong to the "Foo" namespace? It makes sense I 
guess... Have you confirmed that this is what happens?

There's a couple things that break with your patch though. Your modification to 
Loader.load() prevents nested classes from loading their native libraries, and 
when executing something like "java -jar javacpp.jar ParentClassName", the 
nested classes don't get built.

Original comment by samuel.a...@gmail.com on 15 Dec 2012 at 2:16

GoogleCodeExporter commented 9 years ago
For the first problem, I guess Loader.load() could search for the first 
enclosing class with a "library" annotation. For the second problem, I guess we 
could use something similar to recursion for packages, but in this case for 
nested classes. This way, we also wouldn't need to add a "recursive" parameter 
to Generator either, I think...

Original comment by samuel.a...@gmail.com on 22 Dec 2012 at 8:07

GoogleCodeExporter commented 9 years ago
It's been a while, but I've come up with something that should satisfy your 
needs and doesn't break existing functionality:
http://code.google.com/p/javacpp/source/detail?r=18b642d647c22a353415fb66545dd5e
66d4b53f5
I think it's pretty neat too, thanks for the idea. I've limited the scope of 
the new functionality to simplify things, specifically:
- Only top-level classes get a library name by default: Nested class must 
specify @Platform(library="..."), which basically becomes a sort of top-level 
class for all other enclosed classes
- Placing that annotation on different /level/ of nested classes results in 
undefined behavior
- We can't define the order in which header files and libraries get included or 
linked (but that was already a limitation with the -o command line option)

Let me know if this works well for you, thanks!

Original comment by samuel.a...@gmail.com on 3 Feb 2013 at 10:11

GoogleCodeExporter commented 9 years ago
OK. I will have a look at it. i'm sorry i didn't have time to handle this
issue. I had to release my conpany's product. But javacpp is a big help for
the product. I appreciate that. If you give me wiki permission, i could
share my experience with android&javacpp.
2013. 2. 3. ���� 7:11�� <javacpp@googlecode.com>���� �ۼ�:

Original comment by noranb...@gmail.com on 3 Feb 2013 at 11:28

GoogleCodeExporter commented 9 years ago
Thanks! I've giving you permission to edit the Wiki pages. Go ahead!

Original comment by samuel.a...@gmail.com on 3 Feb 2013 at 12:21

GoogleCodeExporter commented 9 years ago
I tried your patch and found that
if @Platform is not specified, cpp is not generated at all.
If I change Loader.appendProperties() to always return true.
jniXXX.cpp is generated for those files.

Is it intentional for return false in Loader.appendProperties()?

Original comment by noranb...@gmail.com on 4 Feb 2013 at 2:39

GoogleCodeExporter commented 9 years ago
Can you give an example of a file where @Platform isn't specified?

Original comment by samuel.a...@gmail.com on 5 Feb 2013 at 1:43

GoogleCodeExporter commented 9 years ago
I made test class just to test this feature without real .c, .h files.
so I didn't use any include files and @Platform.
public class Obj1 {
    public Obj1 {
        allocate();
    }
    private void allocate();
}

But...to come to think of it,
it would be impossible to not using @Platform in real
because they had to include some header file.

Then I think your logic (No @Platform, No generating) is fine.
I will try on more real code.

Original comment by icemins...@gmail.com on 5 Feb 2013 at 2:28

GoogleCodeExporter commented 9 years ago
I tested with real code. It worked well. My test was...

Test1 : using -o option
C:\> java -jar libs\javacpp.jar -cp bin\classes -d jni -nocompile 
com.google.leveldb.DB com.google.leveldb.Iterator com.google.leveldb.Options 
com.google.leveldb.ReadOptions com.google.leveldb.Slice 
com.google.leveldb.Status com.google.leveldb.StdStringPointer 
com.google.leveldb.WriteBatch com.google.leveldb.WriteOptions -o jniLevelDB 
C:\> java -jar libs\javacpp.jar -cp bin\classes -d jni -nocompile 
com.google.leveldb.AndroidEclairEnv
C:\> java -jar libs\javacpp.jar -cp bin\classes -d jni -nocompile 
com.google.leveldb.AndroidFroyoEnv

Test2 : using @Platform(library=...)
C:\> java -jar libs\javacpp.jar -cp bin\classes -d jni -nocompile 
com.google.leveldb.*
(AndroidXXXEnv.java class has @Platform(include="xxx") but no library specified 
and
 all other classes have @Platform(library="jniLevelDB", ...))

-> Success. Both Test1 and Test2 generates same jniLevelDB.cpp, 
jniAndroidEclairEnv.cpp, jniAndroidFroyoEnv.cpp.

Good job~ :)

I ran more tests.

Test3 : same as Test2 except the last argument is package, not classes.
C:\> java -jar libs\javacpp.jar -cp bin\classes -d jni -nocompile 
com.google.leveldb
Warning: Could not find class com.google.leveldb: java.lang.ClassNotFoundExcepti
on: com.google.leveldb
-> Fail. No cpp is generated. I seems package name doesn't work.

Test4 : similar to Test2 but I moved every AndroidXXXEnv classes into Env's 
inner classes because AndroidXXXEnv is only used in Env class.(Top-level Env 
class doesn't have @Platform.)

class Env extends Pointer {
  static Env getAndroidEnv() {return proper AndroidXXXEnv}
  ...
  @Platform(include="...")
  private static class AndroidEclairEnv extends Env {...}  
  @Platform(include="...")
  private static class AndroidFroyoEnv extends Env {...}
  ...
}
-> Fail but expected. jniAndroidXXXEnv.cpp was not generated. According to the 
limit 'Only top-level classes get a library name by default: Nested class must 
specify @Platform(library="...")', this is a expected result. but... because I 
didn't use @Platform at top-level class, I think user would want same result as 
Test5.

Test5 : same as Test4 but I added library="jniAndroidXXXEnv" for each inner 
class.
class Env extends Pointer {
  static Env getAndroidEnv() {return proper AndroidXXXEnv}
  ...
  @Platform(library="jniAndroidEclairEnv", include="xxx")
  private static class AndroidEclairEnv extends Env {...}  
  @Platform(library="jniAndroidFroyoEnv", include="xxx")
  private static class AndroidFroyoEnv extends Env {...}
  ...
}
-> Success. jniAndroidEclairEnv.cpp, jniAndroidFroyoEnv.cpp are generated.

Original comment by noranb...@gmail.com on 5 Feb 2013 at 2:23

GoogleCodeExporter commented 9 years ago
You're right, that limitation doesn't make much sense. I've fixed that and also 
found a way to make the code a bit better:
http://code.google.com/p/javacpp/source/detail?r=21a7a204df4821a80b3dab14f0bf506
6c85c7258

Thanks for testing!

BTW, if you wish to contribute some unit tests, please let me know. I'd give 
you access to the repository so you could upload them there.

Original comment by samuel.a...@gmail.com on 7 Feb 2013 at 1:50

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Test 4 is succeeded as well as Test 1, 2, 5. Hurray~
How about Test3 (package is given)? '.*' is always necessary?
If so, it would better specified in Usage.

I'm not that unit-test-familiar... so it's ok :)

Original comment by noranb...@gmail.com on 8 Feb 2013 at 8:51

GoogleCodeExporter commented 9 years ago
Yes, we need to append ".*", to be consistent with how the `import` statement 
works in the Java language.

About unit tests, you just need to make little tests like you are doing right 
now, put them in the test directory, and that's it! Then they run automatically 
on every build, to make sure they never break.

Original comment by samuel.a...@gmail.com on 8 Feb 2013 at 1:39

GoogleCodeExporter commented 9 years ago
I didn't notice I was missing some instructions for package names. I'll add 
"package (suffixed with .* or .**)" to the "Usage:" line, thanks!

Original comment by samuel.a...@gmail.com on 9 Feb 2013 at 6:47

GoogleCodeExporter commented 9 years ago
Version 0.4 has been released with those changes, enjoy!

Original comment by samuel.a...@gmail.com on 3 Mar 2013 at 11:57