Add --launcher.noRestart option to launcher.

[dhendriks]

This adds a -norestart option to the launcher. When the option is used, it suppresses the restart behavior of exit codes 23 and 24, such that an IApplication can return all exit codes if the restart behavior is not desired.

The diff looks more complicated than it needs to. In eclipse.c, the changes to the switch statement only involve an extra if around the body of two cases, with a comment at the end to explain the fall-through. The bodies of these two new if statements contain the original unmodified original code of the cases, indented one extra level.

See also the discussion at https://github.com/eclipse-equinox/equinox/discussions/378.

[github-actions[bot]]

Test Results

  660 files  ± 0    660 suites  ±0   1h 15m 36s :stopwatch: + 4m 37s 2 201 tests + 6  2 154 :white_check_mark: + 6   47 :zzz: ±0  0 :x: ±0  6 747 runs  +18  6 604 :white_check_mark: +18  143 :zzz: ±0  0 :x: ±0 

Results for commit 14076d9e. ± Comparison against base commit 95971b5c.

:recycle: This comment has been updated with latest results.

[merks]

@HannesWell

Does this look okay to you? Will anything special need to be done to actually build new launchers or will that "just work" given all your build improvements?

@tjwatson

Do you think something with deeper knowledge about the launcher needs to review this. It appears okay to me...

[HannesWell]

Will anything special need to be done to actually build new launchers or will that "just work" given all your build improvements?

The GitHub workflows already pick up the change and build and test the new code. I'm currently working on https://github.com/eclipse-equinox/equinox/issues/575 and hope to complete this within the next one or two weeks. Actually this would be a nice first real-world use and test case. So unless it is urgent, it would be great if we could await the completion of https://github.com/eclipse-equinox/equinox/issues/575 before submitting this which would also avoid the need to subsequently trigger the launcher build manually.

Does this look okay to you?

I'm not very familiar with the code itself, but what I see looks sane to me.

@dhendriks it would be great if you could add a test-case to the existing equinox.launcher.tests. @umairsair could you give some advice how to test this best?

[umairsair]

@umairsair could you give some advice how to test this best?

I think following three tests should be added.. I haven't run the tests though..

    @Test
    void test_appTerminatesWithNoRestartAndEXIT_OK() throws IOException, InterruptedException {
        test_norestart(EXIT_OK);
    }

    @Test
    void test_appTerminatesWithNoRestartAndEXIT_RESTART() throws IOException, InterruptedException {
        test_norestart(EXIT_RESTART);
    }

    @Test
    void test_appTerminatesWithNoRestartAndEXIT_RELAUNCH() throws IOException, InterruptedException {
        test_norestart(EXIT_RELAUNCH);
    }

    private void test_norestart(int exitCode) throws IOException, InterruptedException {
        writeEclipseIni(DEFAULT_ECLIPSE_INI_CONTENT);

        Process launcherProcess = startEclipseLauncher(List.of("-norestart"));

        Socket socket = server.accept();

        List<String> appArgs = new ArrayList<>();
        analyzeLaunchedTestApp(socket, appArgs, null, exitCode);

        // Make sure -norestart arg is picked
        assertTrue(appArgs.contains("-norestart"));
        // Make sure launcher exited with expected exit value
        launcherProcess.waitFor(5, TimeUnit.SECONDS);
        assertEquals(exitCode, launcherProcess.exitValue());
        try {
            server.accept();
            fail("New eclipse started even with -norestart arg and exit code " + exitCode):
        } catch (SocketTimeoutException e) {
            // No new instance launched
            return;
        }
    }

[dhendriks]

@umairsair could you give some advice how to test this best?

I think following three tests should be added.. I haven't run the tests though..

I added the tests. Let's see if they run OK.

[dhendriks]

I'm currently working on #575 and hope to complete this within the next one or two weeks. Actually this would be a nice first real-world use and test case. So unless it is urgent, it would be great if we could await the completion of #575 before submitting this which would also avoid the need to subsequently trigger the launcher build manually.

Let's try to get this in a good shape to be merged now already. For me it is OK to then merge in a few weeks.

[umairsair]

so we are not passing -norestart as parameter to java application, see getVMCommand(..), thats why following assertion is failing..

assertTrue(appArgs.contains("-norestart"));

IMO we should pass it to application so that if java application wants to make any decision based on -norestart arg..

[laeubi]

IMO we should pass it to application so that if java application wants to make any decision based on -norestart arg..

I just wanted to note that one should not confuse JVM argument, Application Arguments and launcher/runtime Arguments.

If you look here then launcher / runtime arguments are usually mapped as java system properties and not passed directly to the invoked application!

[dhendriks]

So, -norestart is a launcher argument, I assume? Then somewhere in eclipse.c I need to add a -Dlauncher.noRestart=true or so? I tried looking for examples of where this is done already, but didn't find any. Could you point me in the right direction?

[umairsair]

see following for reference

https://github.com/eclipse-equinox/equinox/blob/49be75d81307d259b2ec5f01493c284253660fe3/features/org.eclipse.equinox.executable.feature/library/eclipse.c#L1158

you can add following code after above line..

if (noRestart)
    (*progArgv)[ dst++ ] = "--launcher.noRestart";

you can create define for --launcher.noRestart in eclipseCommon.h and update the tests accordingly to check for --launcher.noRestart

[dhendriks]

see following for reference [...] you can create define for --launcher.noRestart in eclipseCommon.h and update the tests accordingly to check for --launcher.noRestart

I added it all:

• Not sure what the define in eclipseCommon.h is for, as it doesn't seem to be used then.
• When updating eclipse.c, I found it difficult to link the argument comment to the argument size computation, so I made them more alike. I also updated the argument description, as it didn't fully match the code (also making it more difficult to keep it in sync).
• I updated the test. Let's see if it succeeds now.

[dhendriks]

@umairsair I implemented it as you indicated. But the tests fail. Partial stack trace:

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
    at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
    at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
    at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
    at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
    at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:31)
    at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:183)
    at org.eclipse.equinox.launcher.tests.LauncherTests.test_norestart(LauncherTests.java:234)
    at org.eclipse.equinox.launcher.tests.LauncherTests.test_appTerminatesWithNoRestartAndEXIT_RESTART(LauncherTests.java:215)

Stdout:

--- start ----
-os
linux
-ws
gtk
-arch
x86_64
-showsplash
-launcher
/tmp/junit15564327931400162296/eclipse/eclipse
-name
Eclipse
--launcher.library
/tmp/junit15564327931400162296/eclipse//plugins/org.eclipse.equinox.launcher/eclipse_11900.so
-startup
/tmp/junit15564327931400162296/eclipse//test.launcher.jar
--launcher.overrideVmargs
-exitdata
4
-norestart
-vm
/opt/tools/java/temurin/jdk-17/latest/bin/java
-vmargs
-Xms40m
-jar
/tmp/junit15564327931400162296/eclipse//test.launcher.jar
--- end ----

It seems -norestart is present, while --launcher.noRestart is not. Do you know what is going on?

[umairsair]

Probably some local setup issue.

I see that tests are not executed for latest commit and requires approval. @merks , @HannesWell can you please approve the run so that we can analyze the latest test results?

[dhendriks]

All 3 tests fail, like this:

org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
    at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
    at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
    at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
    at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
    at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:31)
    at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:183)
    at org.eclipse.equinox.launcher.tests.LauncherTests.test_norestart(LauncherTests.java:234)
    at org.eclipse.equinox.launcher.tests.LauncherTests.test_appTerminatesWithNoRestartAndEXIT_OK(LauncherTests.java:210)

So, on the assertion:

assertTrue(appArgs.contains("--launcher.noRestart"));

I looked at the code, and the changes I did previously. I have to clue what to do next. Did I not change it correctly, to pass that argument on from the launcher to the application?

[umairsair]

last run tells that tests on windows are unstable but failing on linux.. on windows, I hope the suggestion I made above will solve it.. for linux, launcher is not exiting.. that needs to be checked..

https://github.com/eclipse-equinox/equinox/pull/595/checks?check_run_id=24349592476

also macos build is failing..

https://github.com/eclipse-equinox/equinox/actions/runs/8833610849/job/24349296333?pr=595

[dhendriks]

macOS build is indeed failing. But it indicates:

Set up JDK 17 Run actions/setup-java@v4 Installed distributions Trying to resolve the latest version from remote Error: Could not find satisfied version for SemVer '8'. Available versions: 22.0.1+8, 22.0.0+36, 21.0.3+9.0.LTS, 21.0.2+13.0.LTS, 21.0.1+12.0.LTS, 21.0.0+35.0.LTS, 20.0.2+9, 20.0.1+9, 20.0.0+36, 19.0.2+7, 19.0.1+10, 19.0.0+36, 18.0.2+101, 18.0.2+9, 18.0.1+10, 18.0.0+36, 17.0.11+9, 17.0.10+7, 17.0.9+9, 17.0.8+101, 17.0.8+7, 17.0.7+7, 17.0.6+10, 17.0.5+8, 17.0.4+101, 17.0.4+8, 17.0.3+7, 17.0.2+8, 17.0.1+12, 17.0.0+35, 11.0.23+9, 11.0.22+7.1, 11.0.22+7, 11.0.21+9, 11.0.20+101, 11.0.20+8, 11.0.19+7, 11.0.18+10, 11.0.17+8, 11.0.16+101, 11.0.16+8, 11.0.15+10

This seems to be a setup thing even before any code from the repo is used, so I doubt it is because of anything I changed.

[dhendriks]

For Linux: There appear to be two runs for each test. Not sure why. The first ones fail like on Windows. The 2nd ones indeed because process hasn't exited. Hopefully this is also fixed by your proposed change (which I applied).

[dhendriks]

@merks / @HannesWell: Can you please approve the new build/test runs?

[dhendriks]

The org.opentest4j.AssertionFailedError: expected: <true> but was: <false> issues have been resolved. Tests were run for all three platforms now. The test_appTerminatesWithNoRestartAndEXIT_OK test now succeeds. The test_appTerminatesWithNoRestartAndEXIT_RESTART and test_appTerminatesWithNoRestartAndEXIT_RELAUNCH tests both fail with the following error on all three platforms:

process has not exited
java.lang.IllegalThreadStateException: process has not exited
    at java.base/java.lang.ProcessImpl.exitValue(ProcessImpl.java:560)
    at org.eclipse.equinox.launcher.tests.LauncherTests.test_norestart(LauncherTests.java:238)

I have no clue how to debug what is going wrong, and why the process does not terminate.

[HannesWell]

Just as a quick note: all the launcher binaries are now build in the Jenkins pipeline if anything changed. Therefore please rebase your future adjustments on the master. Also please squash all your commits into one with a message that covers the total change (there is no need to record the development loops in git)

I'm not very familiar with the code itself, but I can try to have a look at this tomorrow.

[HannesWell]

In case it is helpful for you: I'm currently working on improving the documentation about how the binaries can be built locally in https://github.com/eclipse-equinox/equinox/pull/640. This also adds a maven build launch config to simply build the binaries for the current platform. With that you could build this change locally and debug the tests. But I can also look into this next week.

[dhendriks]

In case it is helpful for you: I'm currently working on improving the documentation about how the binaries can be built locally in #640. This also adds a maven build launch config to simply build the binaries for the current platform. With that you could build this change locally and debug the tests. But I can also look into this next week.

Sure, that would be / have been helpful. But, the current build failures/aborts do not seem related to this.

[dhendriks]

I have not yet read to this entire thread, but my first impression when looking into the change itself is that in case a no-restart is present the code should fall through to the default branch and tread the exit code as error (which it in fact is, isn't it)?

I completely agree. I had that at first. @umairsair didn't think that was correct. I tried to argue for keeping it, but gave up. It's been 2 months since I created this merge request! It is not easy to adapt this stuff and know what to adapt exactly, when you're not familiar with this code base, build infrastructure, test setup, etc. I'm happy you now agree with me. I want to only skip the restart behavior, making it be treated as any other non-zero exit code. That's all I'm trying to do.

I adapted the code to fall-through again. I also added the --launcher.suppressErrors argument, as suggested. Can you approve another build, to see how this works out now?

[dhendriks]

I adapted the code to fall-through again. I also added the --launcher.suppressErrors argument, as suggested. Can you approve another build, to see how this works out now?

It works! The build and all tests related to this PR succeed!

But, there is one test that fails: testCoordinatedConfigurationOnBeforeRegisteredManagedService (org.osgi.test.cases.cm.junit.CMCoordinationTestCase) failed. Does anyone know what this test tests, where it comes from, and what it has to do with the changes in this branch?

[laeubi]

The test failure is "normal" see:

• https://github.com/osgi/osgi/issues/718

[merks]

@tjwatson

Do you plan to review?

I think @umairsair is probably the most qualified having working on details not so long ago...

In any case, early in the release cycle is a good ti to merge...

[dhendriks]

[...] In general this change looks fine to me now. But I think we should should test cases for the following cases:

• test_norestart with some other error code, e.g. 100 with and without -norestart being passed.
• test that restart and relaunch still work as expected when -norestart is not supplied.

I added tests for exit code 100, with and without -norestart. I also added a test for exit code zero without -norestart.

There are already tests for testing the restart/relaunch behaviors (exit codes 23 and 24). They restart/relaunch even multiple times, etc. They don't supply -norestart. So I think that is already covered by existing tests.

Probably somebody needs to approve another build to see if the new/changed tests succeed.

Btw. was it already discussed if the flag should be named -norestart or --launcher.norestart? From the top of my head I don't remember the difference, but IIRC there is one altough on first sight, it is not clear when --launcher is prefixed.

We have both -norestart and --launcher.noRestart for the app and launcher. I was advised to use these names. Let me know if it needs to be changed.

Furthermore could you please move all the version bumps into a separate first commit of this PR with message Version bumps for 4.33 stream to make it easier to find. I know I have asked you to squash the changes before, but this is different since it separates the outcome of the PR.

I moved all separate changes to separate commits.

A review from somebody who is more an expert at the launcher code would still be welcome.

You asked earlier if @umairsair and @tjwatson could take a look. Maybe they can now have a look?

[umairsair]

I have not yet read to this entire thread, but my first impression when looking into the change itself is that in case a no-restart is present the code should fall through to the default branch and tread the exit code as error (which it in fact is, isn't it)?

I differ here.. consider the case of eclipse where user passed -norestart to eclipse launcher and later used restart option from IDE.. in this case, the shared buffer will contain the launcher args which will be appearing as error pop-up on restart..

if we really want to go this route, we should add support in equinox application as well to either hide restart option or give error to user that this action is unavailable..

[dhendriks]

I have not yet read to this entire thread, but my first impression when looking into the change itself is that in case a no-restart is present the code should fall through to the default branch and tread the exit code as error (which it in fact is, isn't it)?

I differ here.. consider the case of eclipse where user passed -norestart to eclipse launcher and later used restart option from IDE.. in this case, the shared buffer will contain the launcher args which will be appearing as error pop-up on restart..

if we really want to go this route, we should add support in equinox application as well to either hide restart option or give error to user that this action is unavailable..

I think that it does not matter for this whether we fall through or handle exit codes 23 and 24 as other exit codes. In both cases, if a user requests a restart in the IDE and we disabled the special handling of these exit codes, then restart does not work.

I don't think the code for the IDE restart command isn't in this repo. Together with other identified points that need to be done in other repos / other pull requests, this gives the following list of next steps after this merge request:

• Adapt equinox application 'restart' command to inform user that action is unavailable if launcher was given the no restart option.
• Update documentation of the launcher for the new no restart option.

@umairsair Is that OK?

[dhendriks]

@tjwatson Could you also still review this pull request?

[dhendriks]

I rebased to fix a merge conflict on version bumps.

[HannesWell]

[...] In general this change looks fine to me now. But I think we should should test cases for the following cases:

• test_norestart with some other error code, e.g. 100 with and without -norestart being passed.
• test that restart and relaunch still work as expected when -norestart is not supplied.

I added tests for exit code 100, with and without -norestart. I also added a test for exit code zero without -norestart.

There are already tests for testing the restart/relaunch behaviors (exit codes 23 and 24). They restart/relaunch even multiple times, etc. They don't supply -norestart. So I think that is already covered by existing tests.

Yes that's indeed already fine. Perfect, thanks for the updates. Having the indentation fix in a separate commit is IMHO a bit too fine-grained. I have now squashed that into the other clean-up commit and moved the addition of the norestart option into the commit that adds the processing (I believe this was unintentional).

Btw. was it already discussed if the flag should be named -norestart or --launcher.norestart? From the top of my head I don't remember the difference, but IIRC there is one altough on first sight, it is not clear when --launcher is prefixed.

We have both -norestart and --launcher.noRestart for the app and launcher. I was advised to use these names. Let me know if it needs to be changed.

I'll look it up. Until know I have not yet fully understand the logic here. Is it necessary to have both defined? @tjwatson can you advice here?

I have not yet read to this entire thread, but my first impression when looking into the change itself is that in case a no-restart is present the code should fall through to the default branch and tread the exit code as error (which it in fact is, isn't it)?

I differ here.. consider the case of eclipse where user passed -norestart to eclipse launcher and later used restart option from IDE.. in this case, the shared buffer will contain the launcher args which will be appearing as error pop-up on restart.. if we really want to go this route, we should add support in equinox application as well to either hide restart option or give error to user that this action is unavailable..

I think that it does not matter for this whether we fall through or handle exit codes 23 and 24 as other exit codes. In both cases, if a user requests a restart in the IDE and we disabled the special handling of these exit codes, then restart does not work.

I don't think the code for the IDE restart command isn't in this repo. Together with other identified points that need to be done in other repos / other pull requests, this gives the following list of next steps after this merge request:

* Adapt equinox application 'restart' command to inform user that action is unavailable if launcher was given the no restart option.

* Update documentation of the launcher for the new no restart option.

I understand @umairsair's point there, but I'm not yet sure about the conclusion. If one passes -norestart it effectively means that these exit code should be treated as errors and as @dhendriks said it means a restart/relaunch does not work. Furthermore I assume that the norestart mode is mainly used for command-line applications and not for applications with an UI, isn't it? Of course the restart menu entry could be disabled in the UI in this case, the code is probably in https://github.com/eclipse-platform/eclipse.platform.ui. But I would consider this as a nice to have but not a strict requirement. Still it should be documented.

@tjwatson do you have an opinion on this topic?

[dhendriks]

@HannesWell, @tjwatson and @umairsair: what is still needed here? Can you take a look?

[dhendriks]

@dhendriks sorry for the delay. I have not found any definitive documentation/guide-line how to name the arguments, but since the argument to add is definitively only handled by the launcher. Therefore I would be in favor of naming the flag --launcher.noRestart, which is similar to for example --launcher.suppressErrors. I have made a few comments below what I think needs to be changed.

Can you please try if that works, i.e. apply the changes and see the results in the CI? If it works I think this can be submitted.

If I understand the current implementation correct we have a -noRestart argument that controls the launcher behavior but only the presence of --launcher.noRestart is passed to the Eclipse application?

The way I understood it, is: There are options for the launcher. They are use short names. When passed on to the application, they get the longer form to indicate it is an option from the launcher, as an application also has options of itself. Hence, in my version, the launcher got a -norestart and the Eclipse application got --launcher.noRestart. There are separate lists in eclipseCommon.h for this very reason, I thought. But, it was already not consistently applied, so I may be misinterpreting the naming idea: I checked the defined constants, and the ones that are options of the launcher have a one space indentation:

 #define CONSOLE            _T_ECLIPSE("-console")
 #define CONSOLELOG         _T_ECLIPSE("-consoleLog")
 #define DEBUG_ARG          _T_ECLIPSE("-debug") // Post-fixing with _ARG. Otherwise it causes failure in macOS build
 #define OS                 _T_ECLIPSE("-os")
 #define OSARCH             _T_ECLIPSE("-arch")
 #define NOSPLASH           _T_ECLIPSE("-nosplash")
 #define NORESTART          _T_ECLIPSE("-norestart")
#define LAUNCHER           _T_ECLIPSE("-launcher")
 #define SHOWSPLASH         _T_ECLIPSE("-showsplash")
#define EXITDATA           _T_ECLIPSE("-exitdata")
 #define STARTUP            _T_ECLIPSE("-startup")
 #define VM                 _T_ECLIPSE("-vm")
 #define WS                 _T_ECLIPSE("-ws")
 #define NAME               _T_ECLIPSE("-name")
#define VMARGS             _T_ECLIPSE("-vmargs")  /* special option processing required */
#define CP                 _T_ECLIPSE("-cp")
#define CLASSPATH          _T_ECLIPSE("-classpath")
#define JAR                _T_ECLIPSE("-jar")
 #define PROTECT            _T_ECLIPSE("-protect")
#define ROOT               _T_ECLIPSE("root")  /* the only level of protection we care now */

 #define OPENFILE           _T_ECLIPSE("--launcher.openFile")
 #define DEFAULTACTION      _T_ECLIPSE("--launcher.defaultAction")
 #define TIMEOUT            _T_ECLIPSE("--launcher.timeout")
 #define LIBRARY            _T_ECLIPSE("--launcher.library")
 #define SUPRESSERRORS      _T_ECLIPSE("--launcher.suppressErrors")
 #define INI                _T_ECLIPSE("--launcher.ini")
 #define APPEND_VMARGS      _T_ECLIPSE("--launcher.appendVmargs")
 #define OVERRIDE_VMARGS    _T_ECLIPSE("--launcher.overrideVmargs")
#define LAUNCHER_NORESTART _T_ECLIPSE("--launcher.noRestart")
 #define SECOND_THREAD      _T_ECLIPSE("--launcher.secondThread")
 #define PERM_GEN           _T_ECLIPSE("--launcher.XXMaxPermSize")
#define OLD_ARGS_START     _T_ECLIPSE("--launcher.oldUserArgsStart")
#define OLD_ARGS_END       _T_ECLIPSE("--launcher.oldUserArgsEnd")
#define SKIP_OLD_ARGS      _T_ECLIPSE("--launcher.skipOldUserArgs")

#define XXPERMGEN          _T_ECLIPSE("-XX:MaxPermSize=")
#define ADDMODULES         _T_ECLIPSE("--add-modules")
#define ACTION_OPENFILE    _T_ECLIPSE("openFile")
 #define GTK_VERSION        _T_ECLIPSE("--launcher.GTK_version")

It gets things from multiple lists, and none of the lists is completely present. So, I guess I also don't get the naming convention.

Your proposal does look weird though, as in eclipse.c there is a list of options for the command line help text, and none of them have --launcher.xxx syntax... Are you sure the names you proposed are a good idea? I personally find the current names more logical.

I have made a few comments below what I think needs to be changed.

That is incomplete, as it does not account for JavaDocs, tests, etc. I can make the necessary changes, but I'm not sure we should, since I don't see the logic of the new names you propose just yet.

[HannesWell]

Thank you for your detailed answer, I have checked also the surrounding code in more detail.

The way I understood it, is: There are options for the launcher. They are use short names. When passed on to the application, they get the longer form to indicate it is an option from the launcher, as an application also has options of itself. Hence, in my version, the launcher got a -norestart and the Eclipse application got --launcher.noRestart. There are separate lists in eclipseCommon.h for this very reason, I thought.

While it is correct that there are options that are only processed by the launcher (and not passed to the application) and options that are only passed to the application (and not processed by the launcher), I didn't find an option that is processed by the launcher and then passed to the app under a different name (maybe there are options that imply other options?). And I would also find it confusing as a user if I specify some option -A on the CLI and get another option -B passed into my application. I think it should really be the same name. Which name that is, is something that can be discussed.

Your proposal does look weird though, as in eclipse.c there is a list of options for the command line help text, and none of them have --launcher.xxx syntax... Are you sure the names you proposed are a good idea? I personally find the current names more logical.

The reasons I find --launcher.norestart more suitable is that is it similar to --launcher.suppressErrors and since both affect a similar area (i.e. the handling of exit codes) I think they should follow the same naming schema. Furthermore the norestart flag is actually just processed by the launcher. It is just passed to the application for informational purpose and that we can disable the restart button. It is similar to the --launcher.library argument, which is also passed through respectively the -launcher argument (even though that doesn't have the --launcher.X prefix again). And because I assume that that the norestart flag is used very often I also don't think it is very important to make it short.

Why do you prefer just -norestart?

The doc gives the full list of supported options: https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fmisc%2Fruntime-options.html The sources for it are in: https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/3bd05fbd8610fa57766a0399b59ac256e5f50424/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/reference/misc/runtime-options.html#L114-L117

Eventually we should update that as well.

There are separate lists in eclipseCommon.h for this very reason, I thought. But, it was already not consistently applied,

Yes it looks like that. For example I cannot find a any definition nor a usage of the -library argument, although it is listed in the Eclipse program launcher argument list in the beginning of eclipse.c. Therefore I have the impression that list is outdated already and since it is only in this c file, I assume it is not 'normative' in any way.

I have made a few comments below what I think needs to be changed.

That is incomplete, as it does not account for JavaDocs, tests, etc.

That's right. I have adapted to and it should succeed now (at least my local tests after a local rebuild did). I hope I didn't forget something.

[dhendriks]

I changed --launcher.norestart disables the restart behavior of exit codes 23 and 24. to --launcher.noRestart disables the restart behavior of exit codes 23 and 24., capitalizing the R of noRestart. With that, it is now consistently --launcher.noRestart.

[dhendriks]

I updated the merge request title to account for now having --launcher.noRestart, rather than -norestart.

[dhendriks]

I think it should really be the same name. Which name that is, is something that can be discussed.

Alright. Lets go with only --launcher.noRestart then.

@HannesWell Probably another build needs to be approved. If that succeeds, can we merge this?

[HannesWell]

The test-failure is unrelated, submitting.

@dhendriks would you be so kind and also provide a PR to add documentation for this new flag next to https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/3bd05fbd8610fa57766a0399b59ac256e5f50424/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/reference/misc/runtime-options.html#L114-L117

Which eventually will be part of https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fmisc%2Fruntime-options.html

[dhendriks]

Thanks for this enhancement and your patience. Also thanks to @umairsair for your help!

Indeed, thanks @umairsair. And thanks to you as well, @HannesWell. And to the others that participated/helped.

[dhendriks]

@dhendriks would you be so kind and also provide a PR to add documentation for this new flag [...]

I created a pull request for it: https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/pull/2195

[merks]

@dhendriks

Thanks for your persistence and patience! 🏆

[dhendriks]

I was looking at how to pick up the option in the IDE, and I'm not sure passing it as an argument is the right way to go. I can't figure out how to get them. Maybe it should be passed as a property, as @laeubi suggested?

@HannesWell What do you think?

[dhendriks]

Ah, they seem to already be available, as Java property eclipse.commands. I can use that.

[dhendriks]

I've created a PR for the workbench restart not being available. See https://github.com/eclipse-platform/eclipse.platform.ui/pull/2125.