Skip to content

Move CSL to buildres/csl to speedup "processResources" during development#7306

Merged
koppor merged 7 commits into
masterfrom
move-csl
Jan 6, 2021
Merged

Move CSL to buildres/csl to speedup "processResources" during development#7306
koppor merged 7 commits into
masterfrom
move-csl

Conversation

@koppor

@koppor koppor commented Jan 6, 2021

Copy link
Copy Markdown
Member

processResources takes approx. 2 minutes at a fresh run on my Samsung 980 Pro. I think, the reason is the 10.000 CSL files and a slow NTFS file system when it comes to small files.

$ gradlew processResources

> Configure project :
Project : => 'org.jabref' Java module

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.5/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1m 59s
1 actionable task: 1 executed

With this patch, it takes 7 seconds.

$ gradlew processResources

> Configure project :
Project : => 'org.jabref' Java module

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.5/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 7s
1 actionable task: 1 executed

I left some CSL files in the normal resources to ensure that testing is still possible.

When a release is build, all CSL files are still copied.

  • Change in CHANGELOG.md described (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.

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 6, 2021
@koppor

koppor commented Jan 6, 2021

Copy link
Copy Markdown
Member Author

Here is what gitk shows for my actions:

grafik

@koppor

koppor commented Jan 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Tested locally using six-eyes-principle. works.

DominikVoigt
DominikVoigt approved these changes Jan 6, 2021
View reviewed changes

@DominikVoigt DominikVoigt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@calixtus

calixtus commented Jan 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Looks (literally) good (g...) to me
https://www.urbandictionary.com/define.php?term=LGTM 😏

@koppor koppor merged commit a91e8b1 into master Jan 6, 2021
@koppor koppor deleted the move-csl branch January 6, 2021 19:53
@koppor

koppor commented Jan 6, 2021

Copy link
Copy Markdown
Member Author

Side note: When copying the csl-locales locally using the Windows Explorer, it also took me more than 10 seconds.

@Siedlerchr

Siedlerchr commented Jan 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Gradlew run now results in error when opening preferences:

Probably sufficicent to copy the acm-siggraph style
URL url = CitationStyle.class.getResource(STYLES_ROOT + "/acm-siggraph.csl");

java.lang.NullPointerException
at java.base/java.util.Objects.requireNonNull(Objects.java:208)
at org.jabref@100.0.0/org.jabref.logic.citationstyle.CitationStyle.discoverCitationStyles(CitationStyle.java:138)
at org.jabref@100.0.0/org.jabref.gui.util.BackgroundTask$1.call(BackgroundTask.java:59)
at org.jabref@100.0.0/org.jabref.gui.util.DefaultTaskExecutor$1.call(DefaultTaskExecutor.java:160)
at javafx.graphics/javafx.concurrent.Task$TaskCallable.call(Task.java:1425)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
at java.base/java.lang.Thread.run(Thread.java:832)

Siedlerchr added a commit that referenced this pull request Jan 7, 2021
@Siedlerchr
Readd ACM style to csl
ea37f99 
JabRef searches this style when loading

Follow up fix for #7306
@Siedlerchr Siedlerchr mentioned this pull request Jan 7, 2021
Closed
5 tasks
@koppor

koppor commented Jan 7, 2021

Copy link
Copy Markdown
Member Author

Fixed at 8c49600. Tested the end-user distribution only back then...

@koppor koppor mentioned this pull request Mar 29, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@calixtus calixtus calixtus approved these changes

+1 more reviewer

@DominikVoigt DominikVoigt DominikVoigt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

4 participants

@koppor @calixtus @Siedlerchr @DominikVoigt