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] Jabrefonline apollo #7798

Closed
wants to merge 13 commits into from
Closed

[WIP] Jabrefonline apollo #7798

wants to merge 13 commits into from

Conversation

Siedlerchr
Copy link
Member

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@Siedlerchr
Copy link
Member Author

@tobiasdiez I theoretically fixed all this modularity stuff, however, now I am having troubles with Kotlin shit


Users/christophs/workspace/jabref/build/generated/source/apollo/main/service/GetDocumentByIdQuery.java:277: error: cannot access Function1
        final GetUserDocumentRaw getUserDocumentRaw = reader.readObject($responseFields[0], new ResponseReader.ObjectReader<GetUserDocumentRaw>() {
                                                            ^
  class file for kotlin.jvm.functions.Function1 not found
/Users/christophs/workspace/jabref/build/generated/source/apollo/main/service/GetDocumentByIdQuery.java:342: error: cannot access Function2
          writer.writeList($responseFields[3], fields, new ResponseWriter.ListWriter() {
                ^

@Siedlerchr
Copy link
Member Author

Screw this Apollo. Isn't there a better client?

@tobiasdiez
Copy link
Member

Should work now...the kotlin reference is a module, so need to be required explicitly in the module spec

@Siedlerchr
Copy link
Member Author

Ah yes thanks! compiles. Now I have to adapt the output package

@tobiasdiez
Copy link
Member

apollographql/apollo-kotlin#2019

Probably just add the current path to the sourceset.

@Siedlerchr
Copy link
Member Author

Now I only need to get it working in Eclipse^^
Eclipse doesn't like the patch modules somehow. Would be interesting if this works in Intellij.

@koppor
Copy link
Member

koppor commented Jun 7, 2021

As far as I understand the motivation, this makes JabRef a server for some online clients.

I would like to take this as a chance to work on a gradle multi module build (follow-up to #3704).

@koppor
Copy link
Member

koppor commented Jun 7, 2021

This PR adds a new binary file:

grafik

I would like to see that file (apollo-api-jvm-2.5.8.jar) being added in a seperate module only - and keep the normal build "clean".

(I assume, the dependency isn't https://mvnrepository.com/artifact/com.apollographql.apollo/apollo-compiler/2.5.8)

@Siedlerchr
Copy link
Member Author

Siedlerchr commented Jun 7, 2021

@koppor This is the dependency https://mvnrepository.com/artifact/com.apollographql.apollo/apollo-api-jvm
The problem is that apollo.runtime is a split package and we patch it with the api module

build.gradle Show resolved Hide resolved
@tobiasdiez
Copy link
Member

tobiasdiez commented Jun 7, 2021

As far as I understand the motivation, this makes JabRef a server for some online clients.

Nope, it makes JabRef online a possible server that JabRef can sync with (in addition to the Postgres, MySql, ... databases we already support). The apollo stuff we add is a client not a server (it consumes the JabRef online api).

@koppor
Copy link
Member

koppor commented Jun 7, 2021

DevCall: Try it out in IntelliJ (and explain to koppor ^^)

@Siedlerchr
Copy link
Member Author

./gradlew generateApolloSources

* upstream/main:
  Refactoring existing unit tests into parametrized tests (#7700)
  Replace <p> in localization by \n (#7279)
  Bump byte-buddy-parent from 1.11.0 to 1.11.1 (#7800)
  Bump org.javamodularity.moduleplugin from 1.8.6 to 1.8.7 (#7799)
  Bump mockito-core from 3.10.0 to 3.11.0 (#7801)
  Bump classgraph from 4.8.106 to 4.8.108 (#7802)
  Update .markdownlint.yml
  Ignore slant.co links
  GitBook: [main] 3 pages and 17 assets modified
  GitBook: [main] one page modified
  GitBook: [main] one page modified
  GitBook: [main] 6 pages and 42 assets modified

# Conflicts:
#	build.gradle
…line

* 'jabrefonline' of github.com:JabRef/jabref:

# Conflicts:
#	build.gradle
@Siedlerchr
Copy link
Member Author

No matter what I try, I can't get the patched module stuff working in Eclipse.

@tobiasdiez
Copy link
Member

So the imports are not working, or what is the problem?

@Siedlerchr
Copy link
Member Author

As this is still heavy WIP I will move this to koppor to

@Siedlerchr Siedlerchr closed this Jun 16, 2021
@Siedlerchr Siedlerchr deleted the jabrefonline branch June 16, 2021 17:21
@Siedlerchr
Copy link
Member Author

Now located at koppor#505

@tobiasdiez
Copy link
Member

I would say we create a very minimal initial version that works, and then take this as the base branch for further PRs.

What is the problem with this branch though? Anything else that is not working than the eclipse integration?

@Siedlerchr
Copy link
Member Author

The key problem is that a) Eclipse is not working with the patched module, it won't compile and can't find the module when it's located in runtime and b) there was also a problem with the mutation code generation. Need to try this out again.

@Siedlerchr
Copy link
Member Author

Siedlerchr commented Jun 16, 2021

As a workaround I tried to manually patch the module like we did with LO before but this did not work
https://bugs.eclipse.org/bugs/show_bug.cgi?id=529923
https://wiki.eclipse.org/Java9/ModularityOptions

@Siedlerchr Siedlerchr mentioned this pull request Jun 19, 2021
6 tasks
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.

3 participants