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

Win32: Fix compatibility with Windows Server 2012, Windows Server 2016, and Windows 8.1 #1568

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

ShadelessFox
Copy link
Contributor

This PR marks the following Win32 functions as dynamic:

  • OpenThemeDataForDpi1
  • GetSystemMetricsForDpi
  • GetThreadDpiAwarenessContext
  • SetThreadDpiAwarenessContext
  • SystemParametersInfoForDpi

It's enough to make SWT run on Windows Server 2016, Windows Server 2012, and Windows 8.1.

The Java code that uses these functions already checks if the OS version is above the minimum required, so no actions should be taken there.

1Although the OpenThemeDataForDpi is documented to be available since Windows Server 2016, it's not available there, or at least not in the LTS version.

Addresses and fixes #1252 and possibly many other issues related to running SWT apps on older versions of Windows.

Copy link
Contributor

Test Results

   483 files  ±0     483 suites  ±0   7m 52s ⏱️ +30s
 4 095 tests ±0   4 085 ✅ ±0   7 💤 ±0  3 ❌ ±0 
16 173 runs  ±0  16 080 ✅ ±0  90 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 0eb7432. ± Comparison against base commit d54cfca.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! It's great to have the compatibility issue addressed.

The proposal looks completely sound and works as expected. It is consistent with the other implementations of dynamically loaded functions. Only concern might be that in case you call a method that is not available there will be no error but you simply get 0 as a result, but I think it's fine to assume that calls to such a function need to be guarded with a version check (like is already done right now).

I would be in favor of merging this as soon as possible to have it in the upcoming release (and still tested for some days before that).

@Phillipus what is your opinion?

@Phillipus
Copy link
Contributor

Phillipus commented Oct 31, 2024

@Phillipus what is your opinion?

As long as it doesn't break anything, fine.

@HeikoKlare
Copy link
Contributor

As long as it doesn't break anything, fine.

I do not see a risk of breaking anything. It just aligns with other dynmically loaded functions and actually corrects how these functions are called: their calls have already been wrapped in version checks but you can never reach this code as platforms not fulfilling these version constraints will not even start the applications (as we know).

@HeikoKlare
Copy link
Contributor

I've retested different scenarios so that all affected calls have been executed. They all work as expected on my system (Windows 11 22631), which provides all those functions.
I have also re-checked all callers and as stated by @ShadelessFox, they are guarded by according version checks (and anyway, things could not get worse in those environments).

I will merge this so that we have some days/weeks of (implicit) testing. In case there are further remarks or concerns, feel free to comment and let's address them as a follow-up.

For failing tests see: #1564

Thanks again, @ShadelessFox!

@HeikoKlare HeikoKlare merged commit c363394 into eclipse-platform:master Oct 31, 2024
6 of 10 checks passed
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.

UnsatisfiedLinkError of swt-win32-4965r8.dll on Windows Server 2008 R2
3 participants