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

Jump to bottom

Fix dependencies #10326

Merged
merged 8 commits into from
Sep 7, 2023
Merged

Fix dependencies #10326

merged 8 commits into from
Sep 7, 2023

Conversation

koppor
Copy link
Member

@koppor koppor commented Sep 5, 2023

This refs #10274 (comment).

I checked the RuntimeClasspath (!) and did not see any asm there. Thus, this does not fix . #10274, but at least improves the state of dependencies:

image

I also removed the task downloadDependencies, because dependency caching works better these days.

Mandatory checks

  • 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@koppor koppor added the dev: code-quality Issues related to code or architecture decisions label Sep 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Your code currently does not meet JabRef's code guidelines. The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.

More information on code quality in JabRef is available at https://devdocs.jabref.org/getting-into-the-code/development-strategy.html.

Siedlerchr
Siedlerchr reviewed Sep 5, 2023
View reviewed changes
build.gradle
@@ -195,7 +200,9 @@ dependencies {
// route all requests to java.util.logging to SLF4J (which in turn routes to tinylog)
implementation 'org.slf4j:jul-to-slf4j:2.0.7'

implementation 'de.undercouch:citeproc-java:3.0.0-beta.2'
implementation('de.undercouch:citeproc-java:3.0.0-beta.2') {
exclude group: 'org.antlr'
Copy link
Member

@Siedlerchr Siedlerchr Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This needs to be carefully tested I remembere that different asm versions were producing troubles when generating CSL citations at runtime

Copy link
Member Author

Choose a reason for hiding this comment

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

My assumptions:

  1. The direct dependencies of build.gradle are loaded first
  2. In case a library with the same group and name exists, it is ignored.

Siedlerchr
Siedlerchr reviewed Sep 5, 2023
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

Tested the build locally. Preview works.

@koppor
Copy link
Member Author

koppor commented Sep 5, 2023

I would like to get this in - to have a better basis to think about a dependency on asm. See #10274 (comment) for a longer discussion.

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 6, 2023
Siedlerchr
Siedlerchr reviewed Sep 6, 2023
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
Siedlerchr
Siedlerchr reviewed Sep 6, 2023
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
@Siedlerchr
Copy link
Member

Seems like we really need to exclude the dependencies
openjfx/javafx-gradle-plugin#155 (comment)

@Siedlerchr
Copy link
Member

CAn you test #10339 (I updated afterburner fx)

@koppor
Copy link
Member Author

koppor commented Sep 7, 2023

CAn you test #10339 (I updated afterburner fx)

I merged updatejfxplugin in here. Downloaded the windows portable, started it. Starts perfectly.

@Siedlerchr
remove maven local
94b400a 
remove exlcude, no longer necessay
Siedlerchr
Siedlerchr previously approved these changes Sep 7, 2023
View reviewed changes
@Siedlerchr
Merge branch 'fix-dependency-conflicts' of github.com:JabRef/jabref i…
f2e5504 
…nto fix-dependency-conflicts

* 'fix-dependency-conflicts' of github.com:JabRef/jabref:
  test updating javafx plugin but throws at runtime
  Revert "Add workaround for IDEA-317606 (#10322)"
  Add workaround for IDEA-317606 (#10322)
  New Crowdin updates (#10335)
  Add change in en.properties as well
  Correction of a typo in LinkedFileViewModel.java
  Fix devcontainer prebuilds (#10333)
  Add requires on org.freedesktop.dbus (#10332)
  Fix hint to menu
@Siedlerchr Siedlerchr enabled auto-merge September 7, 2023 16:05
@Siedlerchr Siedlerchr added this pull request to the merge queue Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023 •
edited
Loading

The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build.

Merged via the queue into main with commit 29f5cae Sep 7, 2023
@Siedlerchr Siedlerchr deleted the fix-dependency-conflicts branch September 7, 2023 16:59
@Siedlerchr Siedlerchr mentioned this pull request Sep 8, 2023
Merged
6 tasks
Siedlerchr added a commit that referenced this pull request Sep 9, 2023
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into addAddContentToImage
0337872 
* upstream/main:
  Replacement localization (#10358)
  Add workaround for IDEA-317606
  Fix CHANGELOG.md linting issues (#10351)
  fix afterburner
  checkstyle
  add changelog
  Fix NPE when adding new aux group
  Add "--verbose" to MV generator (#10347)
  Fix dependencies (#10326)
  Add new openrewrite rules (#10342)
  New Crowdin updates (#10344)
  Rework duplicate checker tests (#10341)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
dev: code-quality Issues related to code or architecture decisions status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@koppor @Siedlerchr