JNI include paths

[Eric Stevenson]

Not exactly a Java question but figured this group would contain the folks who care about JNI the most.

crbug.com/964169

Due to some subtle build failures that are more commonly hit now that we're breaking apart chrome_java, we want to refactor JNI includes to use the full header path. This will require a mass conversion, so wanted to get some input before going through with it.

Here's what base_jni_headers would look like with these includes: https://chromium-review.googlesource.com/c/chromium/src/+/1634619.

Any objections?

Thanks,

Eric

[Nate Fischer]

The "base/" prefix seems to me like an improvement - it's clearer that ImportantFileWriterAndroid_jni.h comes from a base/ Java file. A bit outside of your immediate goal, it might be nice if the whole file path looked sort-of like the Java file path, ex. "#include base/android/java/src/org/chromium/base/ImportantFileWriterAndroid_jni.h". This is might be too verbose, but has the advantage that it's clearer where the JNI file was generated from (that's sort-of answered on this line).

I don't care about git-churn on #include lines (I seldom use those for interesting blame-based investigations), so your proposal seems OK to me (perhaps others will see problems I've overlooked).

Nate Fischer | Software Engineer | ntf...@google.com

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To post to this group, send email to ja...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CACO9coaiVjpg-dV7o2O64BfqVf%3DH5hNM8yMXF-J8S1nVO3DtgQ%40mail.gmail.com.

[Yaron Friedman]

I'm supportive as well. I think it was always a bug to have this be relative.

I tend to agree with Nate that it'd be nice to point to the java but it has the downside of changing the package of the java file would require changing the C++ import.

Another consideration would be to let the rule specify a path rather than using the target name so that you can change the name in the GN file without having to touch each file.

I'm mostly ambivalent about these choices tough so if you have a preference, go with it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CACiwn8YCVhcVVCONZMgR7woXNt8sEmeDH8%3Dz7rreZzQarjKxWg%40mail.gmail.com.

[David Trainor]

Don't have much to add, but I'm in favor of this kind of change too.  In general I don't mind overly verbose include paths and wouldn't mind a large change to just migrate them over.  But I'd be fine with either of the approaches mentioned.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAC6Qbv_-XxU%3Dp0Rm-f%2BM2Us%2BYonAiHxJkyCN9nvnT6bHhH5_ig%40mail.gmail.com.

[Eric Stevenson]

It may be a bit misleading to use the Java file path since it'll point to a src/ directory that actually exists, but the .h file won't be there. Having the jni target name in the path also makes it more clear that the file is generated imo.

What do you think about using the convention I suggested (#include "base/base_jni_headers/ImportantFileWriterAndroid_jni.h") but then modifying the comment in the generated file to include the actual path of the Java source file?

[David Trainor]

On Thu, May 30, 2019 at 9:48 AM Eric Stevenson <estev...@chromium.org> wrote:

It may be a bit misleading to use the Java file path since it'll point to a src/ directory that actually exists, but the .h file won't be there. Having the jni target name in the path also makes it more clear that the file is generated imo.

What do you think about using the convention I suggested (#include "base/base_jni_headers/ImportantFileWriterAndroid_jni.h") but then modifying the comment in the generated file to include the actual path of the Java source file?

Is that using the gn target as the path name?  IIf so, hat would happen if (when) we get more fine grained gn targets in subdirectories?  I feel like some targets in components are just "jni_headers" which would be okay as long as it didn't end up being: "components/jni_headers" and was "components/featurex/jni_headers".  If not, ignore me :).

Can we use the class package as the file path instead of the actual java file path?  "base/org/chromium/...."?  I'm ok with this requiring changing native c++ imports if the package changes.  We have to do that for all Java imports too anyway in this case.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CACO9cobdM7wC282e5HSoL-w7hore%2BE%3DEi-npTuRBR_B96Tn0QA%40mail.gmail.com.

[Tommy Nyquist]

I just want to echo other comments here. Overly long include paths are totally fine with me.

A) For the initial example of base/jni_headers, I agree with David, that for this to really help, it has to translate: //path/to/directory:my_jni_headers into path/to/directory/my_jni_headers/MyJavaClass_jni.h. If it does that, I think it's a great change.

B) For the other suggestions of having to update the include path if you move Java packages, I think that's totally reasonable to require.

I don't really have a strong opinion as to what makes the most sense of the two, and I think I'd be OK with any of them. With the package name, you can at least go from that into the GN file and find the Java file.

I'd expect most people wanting to find the Java file and not the GN target though, so if you pressed me for a preference, it'd be to use the Java package directory structure.

Anyway, this is a great initiative! Thanks for looking into it!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAAUoNbP_taCDobrJXHdyp_vQLphBSP8K-YezmYEd%2BN5_Lv8Lfw%40mail.gmail.com.

[Eric Stevenson]

Sorry for the delay here. Put up a CL to use java package paths for the generated header files. https://chromium-review.googlesource.com/c/chromium/src/+/1634619 shows what //base jni headers looks like with the new naming scheme.

Ex.

Before: #include "jni/ApkAssets_jni.h"

After:    #include "base/org/chromium/base/ApkAssets_jni.h"

Will move forward with the mass conversion if there are no objections!

[Tommy Nyquist]

That looks reasonable to me.

One question I have is whether we'd want:
#include "base/jni/org/chromium/base/ApkAssets_jni.h"

instead to make it 100% clear that it's JNI related?

I mean, the file name suffix also mentions that, but it's kind of at the end of a long thing :-) Thoughts?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CACO9cob4Lw0Amb2C2WwTwXp%2BAw-HPF%3D9Q8so1rPT6q0dY1%2BmrA%40mail.gmail.com.

[Eric Stevenson]

I tried the approach I mentioned above and it doesn't look great for //chrome jni headers: https://chromium-review.googlesource.com/c/chromium/src/+/1644999. The long includes look pretty bad in gerrit imo. 

We've already discussed most of these, but I thought I'd clearly list all the options with examples for comparisons sake. I'll defer to those who have already commented on this thread to decide what is best since I don't do much C++ development myself.  

1. target_dir/target_name/ClassName_jni.h

#include "base/base_jni_headers/ApkAssets_jni.h"

#include "chrome/android/features/autofill_assistant/jni_headers/AssistantOverlayModel_jni.h"

2. target_dir/Package Path/ClassName_jni.h

#include "base/org/chromium/base/ApkAssets_jni.h"

#include "chrome/android/features/autofill_assistant/org/chromium/chrome/browser/autofill_assistant/overlay/AssistantOverlayModel_jni.h"

3. Package Path/ClassName_jni.h

#include "org/chromium/base/ApkAssets_jni.h"

#include "org/chromium/chrome/browser/autofill_assistant/overlay/AssistantOverlayModel_jni.h"

A caveat of this option is that our code base will not be allowed to contain any duplicate java package/class name pairs for classes that use JNI (to avoid duplicate build outputs).

4. Java Source Path/ClassName_jni.h

#include "base/android/java/src/org/chromium/base/ApkAssets_jni.h"

#include "chrome/android/features/autofill_assistant/java/src/org/chromium/chrome/browser/autofill_assistant/overlay/AssistantOverlayModel_jni.h"

For most of these options, we'll probably have to tweak the DEPS include rules to allow all includes that match ".*_jni.h". 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAA6XDjMRiEVsAo_kiBxkC8V5wHzZbikz9T07mYVtdFXA4c7i9A%40mail.gmail.com.

[Nate Fischer]

> A caveat of this option is that our code base will not be allowed to contain any duplicate java package/class name pairs for classes that use JNI (to avoid duplicate build outputs).

IIRC we have already have a presubmit to discourage duplicate fully-qualified-classes (regardless of whether they use JNI) (crbug/778442). The list of exceptions is fairly minimal.

I think I have a slight preference for "Package Path/ClassName_jni.h" because it hints for how to find the Java file and it's fairly concise. I think "target_dir/Package Path/ClassName_jni.h" (I think that's crrev/c/1634619?) is also OK, although I agree it looks a bit verbose for the chrome/ files.

Nate Fischer | Software Engineer | ntf...@google.com

[Andrew Grieve]

Just back from vacation :)

I like having the java package in the path as well, but I don't think it's technically feasible without explicitly telling GN what the package is for each source file (or by using an exec_script). GN needs to list the outputs of actions, and it doesn't know the package given the source path. We could use the full source path (option 4), but I'd also want $target_gen_dir in there, since that allows DEPS file to work, and follows the very common and useful heuristic that you can figure out what target generates a file based on its path.

We could do $target_gen_dir/$root_relative_source_path, but I think the double-encoding of the root of the paths will look annoying (e.g. #include "chrome/android/features/autofill_assistant/chrome/android/features/autofill_assistant/src/org/chromium/..." . We can't just do $target_gen_dir/$relative_source_path because source paths can start with "../".

I think Option 1 is the best. We could maybe also take the opportunity to rename all generate_jni() targets to be "jni_headers", so that paths usually look like $target_gen_dir/jni_headers/...

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CACiwn8Zzs%2BLnaqaKp%3DsmV9LT8iQcwKxNMw9ouiQ3YwCLty0_tQ%40mail.gmail.com.

[Tommy Nyquist]

Option 1) sounds good to me.

[David Trainor]

+1!