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

[WIP] Modularize JabRef with JPMS #4230

Closed
wants to merge 16 commits into from

Conversation

florian-beetz
Copy link
Contributor

This splits JabRef into multiple JPMS modules. (see #3704)

JabRef runs with these changes, but there are some issues:

  • modernizer and checkstyle Gradle plugins don't seem to play nice with multi-module builds
  • Something seems to screw up logging for tests (ClassCastException: org.slf4j/org.slf4j.helpers.NOPLoggerFactory cannot be cast to org.gradle.internal.logging.slf4j.OutputEventListenerBackedLoggerContext)

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

florian-beetz and others added 3 commits July 23, 2018 13:43
* Fix fetcher tests

remove uncessary logging in l10n which makes fetcher test fail

ISBN tests still fail both
Fix arxiv tests, eprint now has url in front

* rework fetcher tests
Fix logging configuration
Fix eprint field of arxiv
Return empty results when 404 or 400 encountered
Add logging in that case

* fix checkstyle

* match both https and http prefixes in arxiv

* inline prefs in setup method

* fix ebook.de test record, as there is a new version of that book entry

* move comment
@Siedlerchr
Copy link
Member

Siedlerchr commented Jul 23, 2018

@florian-beetz I just fixed some issue with the test xml config part of my fetcher fixes (it was named wrong):
log4.xml -> log4j-test.xml
d5d67d7#diff-91cb0a5b893efefa2c0dbcedbe1a0f85

@florian-beetz
Copy link
Contributor Author

Just tested it with this merged, but I have the same issue.

Here is a full log.

Testing started at 16:18 ...
16:18:58: Executing task 'check --stacktrace'...

> Task :buildSrc:compileJava NO-SOURCE
> Task :buildSrc:compileGroovy UP-TO-DATE
> Task :buildSrc:processResources NO-SOURCE
> Task :buildSrc:classes UP-TO-DATE
> Task :buildSrc:jar UP-TO-DATE
> Task :buildSrc:assemble UP-TO-DATE
> Task :buildSrc:compileTestJava NO-SOURCE
> Task :buildSrc:compileTestGroovy NO-SOURCE
> Task :buildSrc:processTestResources NO-SOURCE
> Task :buildSrc:testClasses UP-TO-DATE
> Task :buildSrc:test NO-SOURCE
> Task :buildSrc:check UP-TO-DATE
> Task :buildSrc:build UP-TO-DATE
> Task :jabref-model:generateSearchGrammarSource UP-TO-DATE
> Task :jabref-model:compileJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
> Task :jabref-model:processResources NO-SOURCE
> Task :jabref-model:classes
> Task :jabref-model:compileTestJava UP-TO-DATE
> Task :jabref-model:processTestResources
> Task :jabref-model:testClasses
> Task :jabref-model:test
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by worker.org.gradle.internal.reflect.JavaMethod (file:/C:/Users/fbeet/.gradle/caches/4.9/workerMain/gradle-worker.jar) to method java.lang.ClassLoader.getPackages()
WARNING: Please consider reporting this to the maintainers of worker.org.gradle.internal.reflect.JavaMethod
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#noProviders for further details.
java.lang.ClassCastException: org.slf4j/org.slf4j.helpers.NOPLoggerFactory cannot be cast to org.gradle.internal.logging.slf4j.OutputEventListenerBackedLoggerContext
	at org.gradle.internal.logging.slf4j.Slf4jLoggingConfigurer.configure(Slf4jLoggingConfigurer.java:42)
	at org.gradle.internal.logging.config.LoggingSystemAdapter.startCapture(LoggingSystemAdapter.java:54)
	at org.gradle.internal.logging.services.DefaultLoggingManager$StartableLoggingSystem.start(DefaultLoggingManager.java:331)
	at org.gradle.internal.logging.services.DefaultLoggingManager.start(DefaultLoggingManager.java:80)
	at org.gradle.internal.logging.services.DefaultLoggingManager.start(DefaultLoggingManager.java:39)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:83)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:64)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:62)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:67)
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by worker.org.gradle.internal.reflect.JavaMethod (file:/C:/Users/fbeet/.gradle/caches/4.9/workerMain/gradle-worker.jar) to method java.lang.ClassLoader.getPackages()
WARNING: Please consider reporting this to the maintainers of worker.org.gradle.internal.reflect.JavaMethod
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#noProviders for further details.
java.lang.ClassCastException: org.slf4j/org.slf4j.helpers.NOPLoggerFactory cannot be cast to org.gradle.internal.logging.slf4j.OutputEventListenerBackedLoggerContext
	at org.gradle.internal.logging.slf4j.Slf4jLoggingConfigurer.configure(Slf4jLoggingConfigurer.java:42)
	at org.gradle.internal.logging.config.LoggingSystemAdapter.startCapture(LoggingSystemAdapter.java:54)
	at org.gradle.internal.logging.services.DefaultLoggingManager$StartableLoggingSystem.start(DefaultLoggingManager.java:331)
	at org.gradle.internal.logging.services.DefaultLoggingManager.start(DefaultLoggingManager.java:80)
	at org.gradle.internal.logging.services.DefaultLoggingManager.start(DefaultLoggingManager.java:39)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:83)
	at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:64)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:62)
	at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:67)
Process 'Gradle Test Executor 2' finished with non-zero exit value 1
org.gradle.process.internal.ExecException: Process 'Gradle Test Executor 2' finished with non-zero exit value 1
	at org.gradle.process.internal.DefaultExecHandle$ExecResultImpl.assertNormalExitValue(DefaultExecHandle.java:395)
	at org.gradle.process.internal.worker.DefaultWorkerProcess.onProcessStop(DefaultWorkerProcess.java:138)
	at org.gradle.process.internal.worker.DefaultWorkerProcess.access$000(DefaultWorkerProcess.java:41)
	at org.gradle.process.internal.worker.DefaultWorkerProcess$1.executionFinished(DefaultWorkerProcess.java:91)
	at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:564)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:35)
	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
	at org.gradle.internal.event.AbstractBroadcastDispatch.dispatch(AbstractBroadcastDispatch.java:42)
	at org.gradle.internal.event.BroadcastDispatch$SingletonDispatch.dispatch(BroadcastDispatch.java:230)
	at org.gradle.internal.event.BroadcastDispatch$SingletonDispatch.dispatch(BroadcastDispatch.java:149)
	at org.gradle.internal.event.ListenerBroadcast.dispatch(ListenerBroadcast.java:140)
	at org.gradle.internal.event.ListenerBroadcast.dispatch(ListenerBroadcast.java:37)
	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:93)
	at com.sun.proxy.$Proxy69.executionFinished(Unknown Source)
	at org.gradle.process.internal.DefaultExecHandle.setEndStateInfo(DefaultExecHandle.java:212)
	at org.gradle.process.internal.DefaultExecHandle.finished(DefaultExecHandle.java:340)
	at org.gradle.process.internal.ExecHandleRunner.completed(ExecHandleRunner.java:102)
	at org.gradle.process.internal.ExecHandleRunner.run(ExecHandleRunner.java:82)
	at org.gradle.internal.operations.CurrentBuildOperationPreservingRunnable.run(CurrentBuildOperationPreservingRunnable.java:42)
	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:63)
	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:46)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:55)
	at java.lang.Thread.run(Thread.java:844)
> Task :jabref-model:test FAILED
Could not execute build using Gradle distribution 'https://services.gradle.org/distributions/gradle-4.9-all.zip'.
16:20:18: Task execution finished 'check --stacktrace'.

@florian-beetz
Copy link
Contributor Author

I just found this and this. Seems to be a Gradle issue actually :(

Siedlerchr and others added 9 commits July 24, 2018 11:01
* update mockito

* update archunit and errorprone

* fix typo
* Replace old fetcher stuff in command line interface

* Remove cli output

* Make help file optional

* Rework web search pane in javafx

* Show web search and group panes by default

* Use better binding and init default fetcher in view model as it should be

* remove obsolete l10n keys
set search button to default so search can be executed by hitting enter

* fix wrong string util class
Adapt the url of returned data
* Make attached files relative to the file directory

Fixes JabRef#4201
Open files with default OS application if no External file type is present

* add changelog

* use method from linked files view model for linked file

* fix opening of file from menu
add monadic binding

* fix variable name
* Fix importer vulnerability
Fixed issue JabRef#4229  where importer was vulnerable to XXE attacks by
disabling DTDs along with adding warning to logger if features are
unavailable. fixes JabRef#4229

* Fix minor code errors and logger optimization
Removed author line in class comment. Reworded CHANGLOG.md. Set
DTD features to individual final static constants. Optimized
logger by parameterizing feature and error.

* Rearrange import statments for project compatibility

* Remove merge artefacts from changelog
(cherry picked from commit 8dce805)
@florian-beetz
Copy link
Contributor Author

I just tried also extracting a logic module, but I encountered a problem:
The class org.jabref.logic.importer.OpenDatabase depends on org.jabref.migrations. I can't move org.jabref.migrations to the logic module, because it depends on the GUI, causing a circular dependency. But I also cannot move org.jabref.logic.importer to the main module, because this package (transitively) depends on half of the logic module, which would also need to be moved to the main module to prevent circular dependencies, but that makes modularizing basically pointless.

Do you have any ideas how this could be refactored to be able to extract the logic module?

@tobiasdiez
Copy link
Member

tobiasdiez commented Aug 1, 2018

Not every class in migrations has a dependency on gui. In particular, the classes used in OpenDatabase only depend on logic and model. Thus, I would propose to split the migration package and move parts of it to logic.migrations and the other parts to gui.migrations. In the future, it would be nice refactor/recode everything so that it fits in logic, but for the moment splitting should be fine.

@florian-beetz
Copy link
Contributor Author

@tobiasdiez Nice, that worked great 👍

@koppor
Copy link
Member

koppor commented Oct 21, 2018

@florian-beetz @tobiasdiez - Since @lenhard pinged us because of architecture. May I ask about the state of this PR? I would not like to put work into the trash. I would really like to keep this going.

@florian-beetz
Copy link
Contributor Author

@koppor This PR is still pretty rough around the edges, and I could not work on this recently.
The biggest problem here right now is that the logging does not work for tests, which means they are currently not runnable in this version.

I also don't know if it makes much sense to continue working on this before #3421 is finished.

@tobiasdiez tobiasdiez self-assigned this Apr 30, 2019
@tobiasdiez
Copy link
Member

I also don't know if it makes much sense to continue working on this before #3421 is finished.

In the devcall, we agreed with this sentiment. The important thing right now is to have a workable Java 9 build, and then worry about the moduliarization of JabRef itself. Thus, I'll put this PR into sleep mode for now. Thanks nonetheless for your work!

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

Successfully merging this pull request may close these issues.

5 participants