Restarting workspace should take into account changes to eclipse.ini

[umairsair]

Background:

There are 3 return codes supported by Eclipse

• 0 - Normal exit
• 23 - Restart the eclipse with previous launcher arguments
• 24 - Restart the eclipse with the arguments provided by Eclipse application itself

The above mentioned problem is not with the exit code 23. The reason is that the eclipse launcher restarts itself in this case, hence eclipse.ini is also reloaded. However eclipse launcher does not restart itself in case of code 24 and parses the arguments provided by Eclipse application in shared memory and simply relaunches java, which means that eclipse.ini will not be reloaded.

For restarts, eclipse should be using code 23 but we had to switch to code 24 to fix following issue.
[WorkbenchLauncher] Restart asks for the workspace to run
And then the fix of following built up on it.
[Workspace chooser] "Older Workspace Version" dialog: "Cancel" button shouldn't close Eclipse but go back to workspace chooser

Fix:

For code 24, the arguments in shared memory are for launcher, so instead of launcher just parsing the args and relaunching java, it should restart the launcher with appropriate arguments. With relaunching the launcher, eclipse.ini will be reloaded like its done for code 23.

Eclipse launcher is updated to relaunch for code 24 as well. Moreover, there are 3 new launcher arguments introduced, 2 are internal to launcher and one is external to launcher and can be set by eclipse application as a relaunch argument.

• --launcher.oldUserArgsStart and --launcher.oldUserArgsEnd: The user arguments passed to eclipse launcher for first start are tracked between these arguments during relaunches. E.g., if eclipse was started as eclipse A B C where A B C are some arguments to launcher and for relaunch, Eclipse application mentioned X Y Z arguments then relaunch command will be eclipse --launcher.oldUserArgsStart A B C --launcher.oldUserArgsEnd A B C X Y Z. Moreover, launcher relaunch will ignore all arguments between and including --launcher.oldUserArgsStart and --launcher.oldUserArgsEnd.
• --launcher.skipOldUserArgs: If eclipse application wants to relaunch with provided arguments only and ignore the older user args then it can mention this argument. So for the case where eclipse was started as eclipse A B C and Eclipse application mentioned X Y Z as relaunch arguments along with --launcher.skipOldUserArgs then relaunch command will be
  eclipse --launcher.oldUserArgsStart A B C --launcher.oldUserArgsEnd X Y Z where launcher relaunch will ignore all arguments between and including --launcher.oldUserArgsStart and --launcher.oldUserArgsEnd.

For restarts, Eclipse application just mentions the -data argument now as relaunch arguments instead of mentioning complete java args. However, user can still mention more arguments for relaunch and Eclipse restart will respect them as well and append the -data argument at the end of user arguments.

See also eclipse-platform/eclipse.platform.ui#1307
Fixes eclipse-equinox/p2#166

[github-actions]

Test Results

   27 files  + 2     27 suites  +2   11m 42s ⏱️ +17s
2 170 tests +14  2 124 ✅ +14  46 💤 ±0  0 ❌ ±0 
2 228 runs  +28  2 182 ✅ +28  46 💤 ±0  0 ❌ ±0 

Results for commit 6e105e3. ± Comparison against base commit 6d32d4f.

♻️ This comment has been updated with latest results.

[umairsair]

@jonahgraham can you please add reviewers?

[umairsair]

@jonahgraham a kind reminder..

[jonahgraham]

@tjwatson can you CC in someone well suited to review this?

[laeubi]

@umairsair this change seem to contain different aspects beside the restarting itself, do you think you can split up some parts in a preliminary PR, this would make this a bit easier to review, e.g. it seem to modify the way how things are build and adds some test as well such things can be maybe reviewed/merged already independent from this.

[laeubi]

This also seem to contain a lot of whitespace changes, also the using of pure maven module seem at least unusual for eclipse projects and might be worth to be explained somewhere in a readme how to develop / test such embedded projects (its currently part of the feature).

[tjwatson]

@tjwatson can you CC in someone well suited to review this?

In the past that has been @sravanlakkimsetti . Sravan, are you aware of someone else ramping up on the launcher? Perhaps @elsazac

[sravanlakkimsetti]

@tjwatson can you CC in someone well suited to review this?

In the past that has been @sravanlakkimsetti . Sravan, are you aware of someone else ramping up on the launcher? Perhaps @elsazac

@gireeshpunathil and @mpalat Can you please take a look?

[gireeshpunathil]

will do. relatively huge change set, so pls expect some time to complete the review; but we will get there. thanks!

[umairsair]

@laeubi , thanks for reviewing the PR..

@umairsair this change seem to contain different aspects beside the restarting itself, do you think you can split up some parts in a preliminary PR, this would make this a bit easier to review, e.g. it seem to modify the way how things are build and adds some test as well such things can be maybe reviewed/merged already independent from this.

everything is related here.. there was no testing framework for launcher testing, so I had to develop it from scratch for testing the changeset done for fixing the restart issue.. and there is no change in current build workflow, the only change is that tests are integrated using new target in the makefile.. we can split the PR but I think it'll simply add overhead because there are only 3 files (one consts file, one small dummy application and one file which actually contains tests) related to testing framework and most tests are related to changeset of this PR..

This also seem to contain a lot of whitespace changes,

Yes, there is one place in eclipse.c where a loop while (running) is removed because of which indentation of many lines is changed..

also the using of pure maven module seem at least unusual for eclipse projects and might be worth to be explained somewhere in a readme how to develop / test such embedded projects (its currently part of the feature).

yeah! I am not sure if we have any testing framework available for any native eclipse stuff? I used this approach to test eclipse IDE and launcher integration using the test java application which mocks eclipse application.. so this is probably very specific case.. I kept it in feature because of two reasons;

1. These tests are completely related to launcher and should not be run in usual equinox builds because we do not generate binaries in those builds..
2. To build launcher binaries in our jenkins build infra (https://ci.eclipse.org/releng/job/Build-eclipse-launcher/), we compress org.eclipse.equinox.executable.feature/library and copy to separate hosts for building the binaries.. since test is also part of library folder, so it'll also be available while building binaries and we can update our jenkins infra to not just build the binaries, but also run the tests..

[umairsair]

Hello Reviewers,

kindly provide your feedback..

[sravanlakkimsetti]

@umairsair Sorry for the delay. I will be reviewing this by end of this week

[umairsair]

a kind reminder!

[sravanlakkimsetti]

started reviewing the native code

[sravanlakkimsetti]

@umairsair I have some queries here.
Did you test the launcher when jvm dll is used instead of java executable as jvm? The reason for this question is to the code under LAUNCH_JNI is modified and just wanted to check whether this flow is tested or not.

You can test this flow by pointing -vm to jvm.dll in bin/server folder

[umairsair]

@umairsair I have some queries here. Did you test the launcher when jvm dll is used instead of java executable as jvm? The reason for this question is to the code under LAUNCH_JNI is modified and just wanted to check whether this flow is tested or not.

You can test this flow by pointing -vm to jvm.dll in bin/server folder

Yes, currently windows tests are using dlls and linux tests are using java executable (default behavior of launcher when vm arg is not mentioned), hence both are being tested.

[sravanlakkimsetti]

In that case lets take this in.
Please rebase your code to latest and increment the min_version to 1900 in

equinox/features/org.eclipse.equinox.executable.feature/library/make_version.mak

Line 16 in 94c7721

min_ver=1804

.

[umairsair]

In that case lets take this in. Please rebase your code to latest and increment the min_version to 1900 in

equinox/features/org.eclipse.equinox.executable.feature/library/make_version.mak

Line 16 in 94c7721

min_ver=1804

.

I'll rebase but this version is updated by jenkins job.
https://ci.eclipse.org/releng/job/Build-eclipse-launcher/
94c7721

[umairsair]

I have updated the version as well and IIUC it has something to do with compatibility.. but kindly explain why are we updating it?

Also, have you reviewed this PR?
eclipse-platform/eclipse.platform.ui#1307

[sravanlakkimsetti]

I have updated the version as well and IIUC it has something to do with compatibility.. but kindly explain why are we updating it?

Also, have you reviewed this PR? eclipse-platform/eclipse.platform.ui#1307

In the version number last two parts are updated by jenkins job. First two digits are manually updated. We do update first two digits for a major change. For this reason I asked you to update the version.

[umairsair]

@sravanlakkimsetti anything pending before we can merge it?

[iloveeclipse]

@sravanlakkimsetti : do we need to trigger a rebuild of native launchers, update aggregator or something like this?

[sravanlakkimsetti]

@sravanlakkimsetti : do we need to trigger a rebuild of native launchers, update aggregator or something like this?

Thats correct. I triggered launcher build. But there is an issue in mac launcher build

@umairsair I got this error Can you please let me know what needs to be done here

00:00:02.968 #if defined(DEBUG) && DEBUG
00:00:02.968                       ^
00:00:02.968 ../eclipseCommon.h:24:39: note: expanded from macro 'DEBUG'
00:00:02.968 #define DEBUG              _T_ECLIPSE("-debug")
00:00:02.968                                       ^
00:00:03.235 eclipseCocoaMain.c:121:5: error: invalid token at start of a preprocessor expression
00:00:03.235 #if DEBUG
00:00:03.235     ^
00:00:03.235 ../eclipseCommon.h:24:39: note: expanded from macro 'DEBUG'
00:00:03.235 #define DEBUG              _T_ECLIPSE("-debug")
00:00:03.235                                       ^
00:00:03.235 eclipseCocoaMain.c:131:5: error: invalid token at start of a preprocessor expression
00:00:03.235 #if DEBUG
00:00:03.235     ^
00:00:03.235 ../eclipseCommon.h:24:39: note: expanded from macro 'DEBUG'
00:00:03.235 #define DEBUG              _T_ECLIPSE("-debug")

[sravanlakkimsetti]

for your reference the build job for launcher https://ci.eclipse.org/releng/view/Launcher/job/Build-eclipse-launcher/93/

[umairsair]

I'll take a look.

[sravanlakkimsetti]

New libraries have been built eclipse-equinox/equinox.binaries@a768faa

[umairsair]

@sravanlakkimsetti following PR also needs to be merged

eclipse-platform/eclipse.platform.ui#1307

Moreover, do we need to add N&N entry somewhere or we are good without it?

[merks]

Although it's kind of a hidden feature, I personally think it's still worth writing an N & N description of this very nice cool improvement!

FYI @jonahgraham

[sravanlakkimsetti]

lets add N&N here https://github.com/eclipse-platform/www.eclipse.org-eclipse/tree/master/news/4.31

[sravanlakkimsetti]

@umairsair Please verify this feature in next build.

[umairsair]

lets add N&N here https://github.com/eclipse-platform/www.eclipse.org-eclipse/tree/master/news/4.31

eclipse-platform/www.eclipse.org-eclipse#113