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.

Sign up for GitHub

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

[WIP] Use v8 engine for CSL #2250

Merged
merged 4 commits into from
Dec 5, 2016
Merged

[WIP] Use v8 engine for CSL #2250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7507fe8
Add a dependency to com.eclipsesource.J2V8 (Java Bindings for V8) in …
shitikanth Nov 8, 2016
b1c4039
Make sure that V8 dependency is available.
shitikanth Nov 16, 2016
5ee4d79
Add entry for J2V8 in external-libraries.txt.
shitikanth Nov 16, 2016
c04f46b
Removed space from licence identifier.
shitikanth Nov 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,26 @@ configurations {
integrationTestRuntime.extendsFrom testRuntime
}

def getV8Dependency() {
def arch = System.getProperty('os.arch')
def dep = null
if (OperatingSystem.current().isWindows()) {
dep = 'win32_x86'
if (arch.equals('amd64')) {
dep += '_64'
}
} else if (OperatingSystem.current().isMacOsX() && (arch.equals('amd64') || arch.equals('x86_64'))) {
dep = 'macosx_x86_64'
} else if (OperatingSystem.current().isLinux() && arch.equals('amd64')) {
dep = 'linux_x86_64'
}
if (dep == null) {
logger.error("Could not find V8 runtime compatible to this system")
return null
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just return an empty string here so that it compiles at other platforms, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that is a good idea, but the jar we produce will still not be platform independent (as @matthiasgeiger mentioned). Maybe we can define different gradle tasks for a platform independent jar and platform specific builds.

What do you guys feel about just directly implementing citeproc natively in Java? I can take it up if you think that it will be worthwhile.

Copy link
Member

Choose a reason for hiding this comment

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

Seeing that (i) the current rendering is very slow and (ii) others will surely use that library too, citeproc natively in Java is a huge win for the community.

I fear, however, that it will be a huge effort and we would also need that effort in JabRef itself.

Copy link

@rmzelle rmzelle Dec 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What do you guys feel about just directly implementing citeproc natively in Java? I can take it up if you think that it will be worthwhile.

Hi all, CSL maintainer here. I have never worked on a citeproc implementation myself, but just a friendly warning that supporting the full CSL 1.0.1 specification is a significant undertaking. I'm cc'ing some developers who might be able to give you some additional pointers/warnings.

(cc @michel-kraemer (citeproc-java), @fbennett (citeproc-js), @inukshuk (citeproc-ruby))

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm this. In fact, I initially thought about creating a pure Java implementation myself, but citeproc-js is so mature and well-maintained, you'll probably never reach the same quality -- at least I would never be able to do so.

Anyway, there are a couple of options to improve the performance of citeproc-java. You already discovered you can use V8, although it isn't documented anywhere. You might be able to improve the performance further by caching the processor instance. But maybe you already do so, I don't know your code well enough. If you're interested we can discuss this topic further. I'd be more than happy to help.

}
return 'com.eclipsesource.j2v8:j2v8_' + dep + ':4.5.0'
}

dependencies {
compile 'com.jgoodies:jgoodies-common:1.8.1'
compile 'com.jgoodies:jgoodies-forms:1.9.0'
Expand Down Expand Up @@ -124,6 +144,11 @@ dependencies {
compile 'org.citationstyles:locales:1.0.1-SNAPSHOT'
compile 'de.undercouch:citeproc-java:1.0.0-SNAPSHOT'

// if available, use platform specific build of V8
if (getV8Dependency() != null) {
compile getV8Dependency()
}

testCompile 'junit:junit:4.12'
testCompile 'org.mockito:mockito-core:2.2.9'
testCompile 'com.github.tomakehurst:wiremock:2.2.2'
Expand Down
5 changes: 5 additions & 0 deletions external-libraries.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,4 +174,9 @@ Project: Citeproc-Java
URL: http://michel-kraemer.github.io/citeproc-java/
Licence: Apache License, Version 2.0

Id: com.eclipse.j2v8
Project: J2V8
URL: https://github.com/eclipsesource/J2V8
Licence: EPL-1.0

The last entry has to end with an empty line. Otherwise the entry is not present in About.html.