Skip to content

Simplify SymLink creation in eclipse.core.tests - #1486

Merged
HannesWell merged 1 commit into
eclipse-platform:masterfrom
HannesWell:simplify-symlink-creation
Oct 25, 2026
Merged

HannesWell merged 1 commit into
eclipse-platform:masterfrom
HannesWell:simplify-symlink-creation

Conversation

@HannesWell

Copy link
Copy Markdown
Member

As discussed in #755 (comment).

@github-actions

github-actions Bot commented Jul 30, 2024 •

Copy link
Copy Markdown
Contributor

Test Results

    54 files  ±0      54 suites  ±0   38m 50s ⏱️ + 2m 17s
 4 888 tests ±0   4 866 ✅ ±0   22 💤 ±0  0 ❌ ±0 
12 555 runs  ±0  12 401 ✅ ±0  154 💤 ±0  0 ❌ ±0 

Results for commit b316530. ± Comparison against base commit 119d38b.

♻️ This comment has been updated with latest results.

@akurtakov
akurtakov force-pushed the simplify-symlink-creation branch from 20654f8 to 1439588 Compare August 9, 2024 18:31
@akurtakov
akurtakov force-pushed the simplify-symlink-creation branch from 1439588 to cd77681 Compare November 26, 2024 20:22
@eclipse-platform-bot

eclipse-platform-bot commented Nov 26, 2024 •

Copy link
Copy Markdown
Contributor

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

runtime/tests/org.eclipse.core.tests.harness/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From 6f0780a1a73d48476dfb80fd369454a4c4bc1203 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <platform-bot@eclipse.org>
Date: Sat, 30 May 2026 04:27:31 +0000
Subject: [PATCH] Version bump(s) for 4.41 stream


diff --git a/runtime/tests/org.eclipse.core.tests.harness/META-INF/MANIFEST.MF b/runtime/tests/org.eclipse.core.tests.harness/META-INF/MANIFEST.MF
index 63dba3b3e1..624a2e0eb5 100644
--- a/runtime/tests/org.eclipse.core.tests.harness/META-INF/MANIFEST.MF
+++ b/runtime/tests/org.eclipse.core.tests.harness/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Eclipse Core Tests Harness
 Bundle-SymbolicName: org.eclipse.core.tests.harness;singleton:=true
-Bundle-Version: 3.17.500.qualifier
+Bundle-Version: 3.17.600.qualifier
 Bundle-Vendor: Eclipse.org
 Export-Package: org.eclipse.core.tests.harness;version="2.0",
  org.eclipse.core.tests.harness.session
-- 
2.54.0

Further information are available in Common Build Issues - Missing version increments.

@akurtakov
akurtakov force-pushed the simplify-symlink-creation branch from 18f2896 to ea84d8f Compare June 6, 2025 08:55
@vogella
vogella force-pushed the simplify-symlink-creation branch 3 times, most recently from 7defb9c to 4e8edfa Compare January 12, 2026 14:38
@vogella

vogella commented Jan 12, 2026

Copy link
Copy Markdown
Contributor

@HannesWell I did the update of your PR, I hope that is ok. Your change looked just to good to be lost.

@HannesWell
HannesWell force-pushed the simplify-symlink-creation branch from 4e8edfa to 465b3d5 Compare January 13, 2026 00:02
@HannesWell

Copy link
Copy Markdown
Member Author

I did the update of your PR, I hope that is ok. Your change looked just to good to be lost.

Yes, of course. Thank you for updating it.

Unfortunately there are (still) failing tests with this and I didn't had the time to investigate the reasons (which is why this staled).
If you can have a look, any help is appreciated.

@HannesWell
HannesWell force-pushed the simplify-symlink-creation branch from 465b3d5 to 6fbe9c4 Compare May 30, 2026 04:22
@ravnskjaer

Copy link
Copy Markdown
Contributor

I’ve looked into the failing SymlinkTest and Bug_185247_recursiveLinks::test6_linkGrandparentTwice (recent master) which also failed for me locally after checking out your branch rebased on master.

Problem
Resolving linkTarget against basedir results in links having absolute targets whereas ln -s/mklink stored the target as given.

Fix
Use Path.of(linkTarget) as the target.

Files.createSymbolicLink throws an IOException instead of the former IllegalStateException when a link can’t be created so canCreateSymLinks() now catches it and return false. I updated the Javadoc.

Branch with your commit plus mine on top of current master: ravnskjaer#1.
All checks are green there on Linux, Windows and macOS.

@HannesWell, should I open a new PR with both commits, or would you rather update this one?
Two possible follow-ups, which I’d do in a separate PR: isDir is now unused, and canCreateSymLinks() no longer throws IOException. Should these be removed or kept?

@HannesWell
HannesWell force-pushed the simplify-symlink-creation branch 2 times, most recently from d3c4ddc to 632ea3e Compare October 24, 2026 10:24
Co-authored-by: Luise Ravnskjaer <13628062+ravnskjaer@users.noreply.github.com>
@HannesWell
HannesWell force-pushed the simplify-symlink-creation branch from 632ea3e to b316530 Compare October 24, 2026 10:47
@HannesWell

Copy link
Copy Markdown
Member Author

Fix
Use Path.of(linkTarget) as the target.

Thank you, that was on point.

Files.createSymbolicLink throws an IOException instead of the former IllegalStateException when a link can’t be created so canCreateSymLinks() now catches it and return false. I updated the Javadoc.

That's right too. To be more precise it throws a FileSystemException. Unfortunately, at least on Windows, only a generic FileSystemException is thrown if the required Admin privileges are missing and it only has a localized message, so nothing we can reliably test on to maintain the previous behavior.

should I open a new PR with both commits, or would you rather update this one?

Since we shouldn't submit broken commits I updated this PR, but added you as Co-author in order to attribute your contribution. I hope this is fine.

isDir is now unused, and canCreateSymLinks() no longer throws IOException. Should these be removed or kept?

It should be removed. If you are interested, please create a follow-up PR for that.
I've refined the exceptions that are thrown to be as specific as possible, so theoretically IOException can still be thrown and it should stay.

Thanks for your input. Now this changed worked for me as expected on Windows when running the tests with and without Admin permissions.

@ravnskjaer

Copy link
Copy Markdown
Contributor

Thanks for folding the fix in and for the co-author credit. Catching FileSystemException makes sense given the Windows behavior. I’ll open a follow-up PR removing isDir once this is merged.

@HannesWell
HannesWell merged commit 1887998 into eclipse-platform:master Oct 25, 2026
18 checks passed
@HannesWell
HannesWell deleted the simplify-symlink-creation branch October 25, 2026 07:52
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.

4 participants