Fix console launcher not consulting eclipse.ini to read vmargs when there is no console window (#117)

[jonathan-meier]

Fixes #117. The fix is to properly propagate the launcher mode from main to the launcher DLL and removing the incomplete isConsoleLauncher function entirely.

[akurtakov]

@niraj-modi would you please review this patch?

[github-actions]

Unit Test Results

     24 files       24 suites   11m 38s ⏱️
2 137 tests 2 092 ✔️ 44 💤 1 ❌
2 181 runs  2 136 ✔️ 44 💤 1 ❌

For more details on these failures, see this check.

Results for commit 27ae5ac.

[akurtakov]

@nnemkin Is it possible that you review this patch ?

[Torbjorn-Svensson]

I just stumbled on this same problem yesterday.
To me, the proposed solution looks fine as it removes the ambiguity for the expected name of the .ini file.

[jonahgraham]

I've had a look too and it LGTM and better than existing solution all around, especially as it fixes the comment "should look for a better way to do this"

Please merge this in at the beginning of the 2023-03 dev cycle. (I'm not an equinox committer)

[github-actions]

Test Results

     24 files       24 suites   11m 23s ⏱️
2 139 tests 2 094 ✔️ 44 💤 1 ❌
2 183 runs  2 138 ✔️ 44 💤 1 ❌

For more details on these failures, see this check.

Results for commit 0258876.

[Torbjorn-Svensson]

@vogella are you planning to do more work/review on this PR or was your force-push just a rebase?
Do you know anyone that can take a look at this PR and hopefully get it merged?

[vogella]

@vogella are you planning to do more work/review on this PR or was your force-push just a rebase? Do you know anyone that can take a look at this PR and hopefully get it merged?

Just a rebate. Can you check the failing tests?

[Torbjorn-Svensson]

@vogella are you planning to do more work/review on this PR or was your force-push just a rebase? Do you know anyone that can take a look at this PR and hopefully get it merged?

Just a rebate. Can you check the failing tests?

I have no idea why the test testCircularityHandling (org.eclipse.equinox.ds.tests.tbc.DSTest) would fail with this change.
To me, this test has nothing to do with what .ini file to read in the eclipsec.exe file. Are you of a different opinion?
Does this work for other PR's?

[iloveeclipse]

Can you check the failing tests?

I'm pretty sure the native libraries code isn't built & tested anyway on PR's, and there are no tests for that change either.

The binaries are supposed to be built manually via magic jenkins jobs, see #16 (comment)

More important question is: has anyone tried to build the code and test if that change actually works?

[Torbjorn-Svensson]

Can you check the failing tests?

I'm pretty sure the native libraries code isn't built & tested anyway on PR's, and there are no tests for that change either.

The binaries are supposed to be built manually via magic jenkins jobs, see #16 (comment)

More important question is: has anyone tried to build the code and test if that change actually works?

No, I've not built it and I don't even have enough permission to do it on the Jenkins job.
All I've done is to review the change and based on that it relies on the define instead of doing a runtime check, makes it less subject to work some time and not some other time.

[Torbjorn-Svensson]

Using the github action approach in #247, I've been able to validate the functionality using the following approach:

1. Created a small console application RCP in that simply prints the value of java property and exits.
2. Modified the eclipse.ini to contain the definition of the property
3. Created a small java application that runs the product exported in step 1.

The java application contains the following code:

public class Main {
    public static void main(String[] args) throws IOException, InterruptedException {
        String path = "C:\\tmp\\eclipse";

        Process proc = new ProcessBuilder(path + "\\eclipsec.exe")
                .directory(new File(path))
                .start();
        
        System.out.println("Exit code: " + proc.waitFor());

        try (BufferedReader is = new BufferedReader(new InputStreamReader(proc.getInputStream()))) {
            String line;
            while ((line = is.readLine()) != null) {
                System.out.println(line);
            }
        }
    }
}

When running the application defined above with the Eclipse 2023-03 SimRel binaries, the property cannot be extracted:

Exit code: 0
Property value is: null

When running it with the binaries produces with pr/118, then I instead get the following result:

Exit code: 0
Property value is: foo

Other than a rebase and removing some trailing whitespace, I would say that this PR is ready for a merge (keep in mind that I'm not an equinox committer).

What's the next step to get this change merged?

[vogella]

Merging based on Torbjorn-Svensson review.

[vogella]

Thanks @jonathan-meier and @Torbjorn-Svensson for working in this and testing it. We (PMC) decided a while ago that should remove the native support for splash screen from the launcher as the assumption was that this would greatly simplify the launcher code. Maybe something you could help with, in case you also think that simplification of the launcher code might be helpful.

[HannesWell]

In the area of the launcher the following would be also an interesting issue.

• Bug 577416 - Unify Console creation and interaction behaviour of Eclipse launcher #249

[umairsair]

See #117 (comment)

is this fix part of latest eclipse releases?

IIUC, the binaries are picked from following location and I don't see a new commit after this fix.. am I missing something here?

https://github.com/eclipse-equinox/equinox.binaries/tree/master/org.eclipse.equinox.executable/bin/win32/win32/x86_64