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

Change location of .tooling #1655

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Change location of .tooling #1655

merged 1 commit into from
Feb 12, 2021

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Feb 2, 2021

Fixes #1654
Java LS and buildship will use $XDG_CACHE_HOME/tooling, ~/.cache/tooling per default

Signed-off-by: Snjezana Peco snjezana.peco@redhat.com

Jorenar
Jorenar reviewed Feb 2, 2021
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/internal/gradle/checksums/WrapperValidator.java Outdated
}

public static File getSha256CacheFile() {
String checksumCache = System.getProperty("gradle.checksum.cacheDir");
File file;
if (checksumCache == null || checksumCache.isEmpty()) {
file = new File(System.getProperty("user.home"), ".tooling/gradle/checksums");
String xdgCache = getXdgCache();
file = new File(xdgCache, "tooling/gradle/versions.json");
Copy link

@Jorenar Jorenar Feb 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't stay checksums instead of versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Fixed

@fbricon
Copy link
Contributor

fbricon commented Feb 2, 2021 •
edited
Loading

XDG_CACHE_HOME is distribution specific. Please use a jdt.ls specific env var

@snjeza
Copy link
Contributor Author

snjeza commented Feb 2, 2021 •
edited
Loading

XDG_CACHE_HOME is distribution specific. Please use a just.ls specific env var

buildship uses it. See

@fbricon
Copy link
Contributor

fbricon commented Feb 2, 2021

ok fair enough

@rgrunber
Copy link
Contributor

rgrunber commented Feb 3, 2021

When I run the build locally on these changes I see :

Results:

Failures: 
  ClasspathUpdateHandlerTest.testClasspathUpdateForGradle:112 
Wanted but not invoked:
connection.sendEventNotification(
    <Capturing argument>
);
-> at org.eclipse.jdt.ls.core.internal.handlers.ClasspathUpdateHandlerTest.testClasspathUpdateForGradle(ClasspathUpdateHandlerTest.java:112)
Actually, there were zero interactions with this mock.

  GradleBuildSupportTest.testUpdate:54->AbstractProjectsManagerBasedTest.assertHasErrors:269 simple-gradle has no errors
  GradleProjectImporterTest.testSettingsGradle:540
  GradleProjectImporterTest.testSubprojects:510
Errors: 
  GradleProjectImporterTest.testDeleteClasspath:437 »  Unresolved compilation pr...

Tests run: 1273, Failures: 4, Errors: 1, Skipped: 12

Build passes without this change. Are you able to reproduce at all ?

@snjeza
Copy link
Contributor Author

snjeza commented Feb 3, 2021

When I run the build locally on these changes I see :

@rgrunber What OS and JDK are you using?

@rgrunber
Copy link
Contributor

rgrunber commented Feb 3, 2021

When I run the build locally on these changes I see :

@rgrunber What OS and JDK are you using?

Fedora release 33 (Thirty Three)
Apache Maven 3.6.3 (Red Hat 3.6.3-5)
Maven home: /usr/share/maven
Java version: 11.0.9.1, vendor: Red Hat, Inc., runtime: /usr/lib/jvm/java-11-openjdk-11.0.9.11-9.fc33.x86_64

@snjeza
Copy link
Contributor Author

snjeza commented Feb 3, 2021

@rgrunber I have reproduced the issue. Checking...

@snjeza
Change location of .tooling
ddb06e1 
Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@snjeza
Copy link
Contributor Author

snjeza commented Feb 12, 2021

@rgrunber could you check this issue again?

@rgrunber
Copy link
Contributor

I re-ran the build and the tests are all passing. Feel free to merge.

@snjeza snjeza merged commit e663804 into eclipse-jdtls:master Feb 12, 2021
@rusty-snake rusty-snake mentioned this pull request Jul 7, 2021
Merged
@snjeza snjeza mentioned this pull request Oct 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jorenar Jorenar Jorenar left review comments

@gorkem gorkem Awaiting requested review from gorkem

@fbricon fbricon Awaiting requested review from fbricon

@rgrunber rgrunber Awaiting requested review from rgrunber

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change location of .m2/ and .tooling/ from HOME
4 participants
@snjeza @fbricon @rgrunber @Jorenar