Add methods to identify the current OS to the class Platform

[fedejeanne]

(fulfill the changes requested in #446 (comment))

Move the methods isLinux(), isWindows() and isMacOSX() (renamed to isMac()) to the new internal class org.eclipse.core.runtime.Platform.OS and add a new method OS.is(String) for legacy/flexibility purposes.

Contributes to #448

[github-actions]

Test Results

       42 files  +     14         42 suites  +14   51m 21s ⏱️ + 14m 7s
  3 777 tests ±       0    3 774 ✔️ +       5    3 💤  - 5  0 ❌ ±0 
11 334 runs  +3 778  11 307 ✔️ +3 771  27 💤 +7  0 ❌ ±0 

Results for commit 2136579. ± Comparison against base commit 9dbc9e4.

♻️ This comment has been updated with latest results.

[akurtakov]

Is there intention for these to be used outside of the tests too?
Also there are some other OS_* constants so maybe having single Platform.isOS(String os_name_constant) would be enough and also be more future proof (e.g. bsd support gets added or next windows no longer be win32 ).

[fedejeanne]

Is there intention for these to be used outside of the tests too?

I haven't changed any other existing calls of the type Platform.getOS().equals(Platform.OS_XYZ) in other productive/tests classes, I wanted to keep the changes to the minimum while getting rid of the utility methods in ResourceTest

Also there are some other OS_* constants so maybe having single Platform.isOS(String os_name_constant) would be enough and also be more future proof (e.g. bsd support gets added or next windows no longer be win32 ).

Good point, I missed those ones. I took your suggestion and replaced the methods with a single method called isOS

[laeubi]

I think isWindows() and alike are still good especially for the case

e.g. bsd support gets added or next windows no longer be win32

so one can then handle the case that isWindows means win32 OR win2050

[fedejeanne]

I think isWindows() and alike are still good especially for the case

e.g. bsd support gets added or next windows no longer be win32

so one can then handle the case that isWindows means win32 OR win2050

This could have unintentional effects if an older version of Windows supports different features than a newer version e.g. here:

eclipse.platform/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/filesystem/SymlinkTest.java

Lines 43 to 44 in 5a3244b

	private static final boolean SYMLINKS_ARE_FIRST_CLASS_FILES_OR_DIRECTORIES = Platform.OS_WIN32
	.equals(Platform.getOS()) ? true : false;

I'd rather keep the API "low level" in this case and -if necessary- provide another method that checks for the OS-"family" in a different PR. What do you think?

[HeikoKlare]

I agree that there should only be one general isOS() method instead of methods for every (currently supported) system.
Some further considerations:

1. I do not agree with the name isOS as it sounds as if it checks whether the given argument is an OS. Something like isExecutedOn() would be easier to comprehend when reading. Another option would be to create a PlatformOS class (or an inner OS class of Platform) encapsulating what is related to the OS on which the platform is executed instead of further extending the already quite large lists of methods provided by Platform.
2. What about representing the OS passed to the new method in a typed way (e.g., as an enum literal)? That would improve the API, as you cannot (accidentally) pass faulty strings to the method with unexpected results. And additions like OS families including multiple OSes would then be possible without changing the method (the method would still accept an OS / OS family) but by representing this as relations between OS / OS family.

[laeubi]

I just wanted to mention the following:

1. Currently it already is generic
2. It is so generic that we have helper methods (name isWindows() / isLinux() / is....) in some codes and a lot of boilerplate code that effectively wanted to test if it is a general windows / linux / ... for very specific cases
3. It was recommended to move these helper methods here as they serve a purpose and are easier to understand

If we now replace the one generic one with the next generic one that only differs a very tiny bit in the call structure Plattform.getOs().equals(SOME_CONSTANT) versus Plattform.isOs(SOME_CONSTANT) this renders the whole proposal a bit useless (from my point of view).

[HeikoKlare]

I see your points. From my point of view, the problem with Plattform.getOs().equals(SOME_CONSTANT) is that some explicit string comparison is used for identifying a platform (which is bad from both a maintability/encapculation and a usability perspective). Instead, Plattform.isOs(SOME_CONSTANT) is capcable of properly encapsulating what is done with the constant to identify the OS. And with respect to usability, having isLinux() instead of isOS(OS_LINUX) or the like is rather a matter of laziness than of proper API design, isn't it?

Still, I do not have a strong opinion on whether to have one or multiple methods. But in case of having one method for every OS, I would still suppose to not bloat up the Platform class with it, but rather have Platform.OS.isLinux() or the like, as it also makes the code easier to understand and provides a bit of separation of different concerns within the Platform class.

[laeubi]

From my point of view, the problem with Plattform.getOs().equals(SOME_CONSTANT) is that some explicit string comparison is used for identifying a platform (which is bad from both a maintability/encapculation and a usability perspective).

At least it is the way if one need maximum flexibility

Instead, Plattform.isOs(SOME_CONSTANT) is capcable of properly encapsulating what is done with the constant to identify the OS

What should be kept in mind is that these constants are already not any raw values, these are already processed (see for example here https://docs.osgi.org/reference/osnames.html for a list of "real" os and their alias) int o a somewhat normalized form, so usually the variants are handled at another place

And with respect to usability, having isLinux() instead of isOS(OS_LINUX) or the like is rather a matter of laziness than of proper API design, isn't it?

I'm not sure if "API design" really applies to this kind of code as we don't will have any competing implementations nor is this a general purpose data carrier or alike the main advantage for me is that one can use that as a boolean expression in lamda filter style methods like Platform::isLinux in contrast to ()->Platform.isOS(Platform.OS_LINUX) and it is much more obvious / shorter then.

Also I'd like to mention that where this is used, we only use it in a very rough form, e.g. should a test run on windows or linux only, or should i try to use a DLL instead of a so file, it is not really meant as a fine grained feature switch that would more need something like is it an Redhat or Debian or Wayland versusx X11 or Windows XP 32 bit versus Windows 11 ARM and so on.

Sadly especially for SWT there are sometimes some quirks where I use a similar check in user-code, also there I only need a very very rough categorization and really don't mind about any future OSes or ancient OSes that where supported 10 years ago ...

[fedejeanne]

So in sum:

• Move the methods to a new inner class Platform.OS to avoid bloating Platform
• Rename isOS to isExecutedOn
• Provide isExecutedOn(SOME_STRING_CONSTANT) as well as isLinux(), isWindows() and isMacOSX() for both flexibility and comfort (in lambda expressions)
• No need to provide "comfort" methods for the deprecated OSs (i.e. no isAIX(), isHPUX etc)
• isXYZ() means "is any type of OS of the family XYZ" (e.g. isWindows() means "is win32 or win2050")

The last point has no implications for this PR since there are no "related" values (constants) for the OSs at the moment, right?

I left out the part about using Enum-Values instead of Strings as parameter because that would mean that one needs to have the latest version of the Enum in the classpath to be able to use the method with newer OSs (or extending/patching the Enum locally). I guess one could provide both approaches (Strings and Enums) but that would complicate the API.

What do you guys think? (👍 / 👎 )

[fedejeanne]

@iloveeclipse which point would you like to change/scratch?

[iloveeclipse]

In almost every test bundle we have some utility class with isWindows() isLinux() and is Mac(), so I don't understand the need for extra complicated constants, long method names etc. So basically what #704 (comment) says.

[fedejeanne]

In almost every test bundle we have some utility class with isWindows() isLinux() and is Mac(), so I don't understand the need for extra complicated constants, long method names etc. So basically what #704 (comment) says.

In that case I humbly request some clarification from @laeubi about what he meant in #704 (comment) because I think I interpreted it completely differently: would you please list your proposals/wishes, @laeubi ?

[HeikoKlare]

Would adding the following methods address all concerns?

• Platform.OS.isLinux()
• Platform.OS.isWindows()
• Platform.OS.isMacOS()
• Platform.OS.is(String OS_CONSTANT) (for checking for other / more specific OSes than with the other methods, as a replacement for Platform.getOS().equals(OS CONSTANT))

[iloveeclipse]

Would adding the following methods address all concerns?

OS.isMacOS() -> OS.isMac() please

But the rest would be OK.

[fedejeanne]

Done in 0af08ad

[HeikoKlare]

For me, changes would be fine as proposed now.

[fedejeanne]

Test failures documented in #715

[fedejeanne]

The pipeline error is unrelated, maybe a connection problem.

[Pipeline] junit
Recording test results
No test report files were found. Configuration error?
Error when executing always post condition:
Also:   hudson.remoting.Channel$CallSiteStackTrace: Remote call to JNLP4-connect connection from 10.40.62.179/10.40.62.179:46258
		at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1784)
		at hudson.remoting.UserRequest$ExceptionResponse.retrieve(UserRequest.java:356)
		at hudson.remoting.Channel.call(Channel.java:1000)
		at hudson.FilePath.act(FilePath.java:1192)
		at hudson.FilePath.act(FilePath.java:1181)
		at hudson.tasks.junit.JUnitParser.parseResult(JUnitParser.java:118)
		at hudson.tasks.junit.JUnitResultArchiver.parse(JUnitResultArchiver.java:157)
		at hudson.tasks.junit.JUnitResultArchiver.parseAndSummarize(JUnitResultArchiver.java:251)
		at hudson.tasks.junit.pipeline.JUnitResultsStepExecution.run(JUnitResultsStepExecution.java:63)
		at hudson.tasks.junit.pipeline.JUnitResultsStepExecution.run(JUnitResultsStepExecution.java:29)
		at org.jenkinsci.plugins.workflow.steps.SynchronousNonBlockingStepExecution.lambda$start$0(SynchronousNonBlockingStepExecution.java:47)
		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:1128)
		at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
		at java.base/java.lang.Thread.run(Thread.java:839)
hudson.AbortException: No test report files were found. Configuration error?
	at hudson.tasks.junit.JUnitParser$ParseResultCallable.invoke(JUnitParser.java:184)
	at hudson.FilePath$FileCallableWrapper.call(FilePath.java:3578)
	at hudson.remoting.UserRequest.perform(UserRequest.java:211)
	at hudson.remoting.UserRequest.perform(UserRequest.java:54)
	at hudson.remoting.Request$2.run(Request.java:377)
	at hudson.remoting.InterceptingExecutorService.lambda$wrap$0(InterceptingExecutorService.java:78)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at hudson.remoting.Engine$1.lambda$newThread$0(Engine.java:125)
	at java.base/java.lang.Thread.run(Thread.java:829)

[HeikoKlare]

I case no one has objections to the current proposal, I propose to merge after the freeze period.

[HeikoKlare]

@laeubi Since you gave so much valuable input to improve the initial proposals, it would be great to have a short notice whether you see your concerns addressed and agree with the current proposal, so that we can merge.

[laeubi]

Looks good.