Add --launcher.noRestart option to launcher. #595
Conversation
features/org.eclipse.equinox.executable.feature/library/eclipse.c
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.equinox.app/src/org/eclipse/equinox/app/IApplication.java
Outdated
Show resolved
Hide resolved
|
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? Do you think something with deeper knowledge about the launcher needs to review this. It appears okay to me... |
HannesWell
force-pushed
the
add-norestart-option-to-launcher
branch
from
2de24d4 to
fb4f95e
Compare
The GitHub workflows already pick up the change and build and test the new code.
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? |
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. |
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. |
|
so we are not passing IMO we should pass it to application so that if java application wants to make any decision based on |
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! |
|
So, |
|
see following for reference you can add following code after above line.. you can create define for |
I added it all:
|
|
@umairsair I implemented it as you indicated. But the tests fail. Partial stack trace: Stdout: It seems |
|
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? |
|
All 3 tests fail, like this: So, on the assertion: 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? |
features/org.eclipse.equinox.executable.feature/library/eclipse.c
Outdated
Show resolved
Hide resolved
|
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 |
| memcpy(relaunchCommand, initialArgv, (initialArgc + 1) * sizeof(_TCHAR*)); | ||
| relaunchCommand[initialArgc] = 0; | ||
| relaunchCommand[0] = program; | ||
| break; |
There was a problem hiding this comment.
the break of case statement should still be outside of if condition, right? otherwise any data put in exitData buffer by the application will be considered error message in default case..
There was a problem hiding this comment.
The essence is:
case RESTART_LAST_EC:
if (!noRestart) {
...
break;
} // else: fall-through to 'default' case.
case RESTART_NEW_EC:
if (!noRestart) {
...
break;
} // else: fall-through to 'default' case.
default: {
...If the exit code is a restart exit code, then if we allow restart (we do not disallow restarting), we do something special (whatever was there already before) and then break. But, if restart is disabled, we instead fall through to the default case, and handle it like any other non-zero exit code.
So, that part still looks good to me.
There was a problem hiding this comment.
otherwise any data put in exitData buffer by the application will be considered error message in default case..
Did you consider this?
There was a problem hiding this comment.
and this is infact the reason of tests failure..
test app sets exit data here in case user selects restart from within application.
and in launcher, as -norestart is mentioned and default is executed, this exitData is shown as error..
There was a problem hiding this comment.
I was trying to only skip the RESTART_* cases, but I think you're saying also exitData may contain something related to restart behavior? I'm really not sure what that variable is about, at all.
eclipse.c has more than 2000 lines. I have to say there is a lot of stuff going on there, and I don't know what half of it means. It is complicated.
Can the default case really be skipped fully? It seems to handle reporting various errors. For instance, the errorMsg == NULL seems relevant, as it reports problems launching Java? Are you sure we can safely skip all of that? I don't want it to print anything in case of a non-zero exit code, but it should probably still report issues for failing to launch Java to begin with.
Should we do if (exitData != NULL) free(exitData); before falling through? And what is getSharedData about? That can also set exitData again?
There was a problem hiding this comment.
I've retriggered the build
There was a problem hiding this comment.
I had to recursively adapt 4 more features. I hope that is all the versions that need updating. Needs another build approval.
There was a problem hiding this comment.
If I understand it correctly, the new tests pass. There is one test that fails: testCoordinatedConfigurationOnBeforeRegisteredManagedService (org.osgi.test.cases.cm.junit.CMCoordinationTestCase) failed. Not sure what that is, or where it comes from, and whether that is even related to the changes I made? Also, 2/23 checks were aborted. I also have no idea why that is.
There was a problem hiding this comment.
The CMCoordinationTestCase failed test also fails in #640 that @HannesWell refers to in a different thread of this PR. I don't think that failure is related to my changes.
I still don't know what caused the 2 other checks to be aborted. Their output doesn't help me much. Although, I see they were 'aborted'; they didn't 'fail'. I also in Eclipse ESCET recently got timeout aborts due to the Jenkins cluster being slower than usual, and I had to increase the timeout there. Could that be the reason here as well?
There was a problem hiding this comment.
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)?
By default the executable pops-up a native error dialog if the app exited with an error code.
Could the test-failure with a fall-through to default be because of that? Because at the server such pop-up is closed by nobody.
Maybe the test would be more correct if you use from the Eclipse runtime options
--launcher.suppressErrors (Executable)
If specified the executable will not display any error or message dialogs. This is useful if the executable is being used in an unattended situation.
Then the error code is just returned by the command and could be checked in the invoking java test code?
|
macOS build is indeed failing. But it indicates: Set up JDK 17 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. |
|
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 |
|
@merks / @HannesWell: Can you please approve the new build/test runs? |
|
The I have no clue how to debug what is going wrong, and why the process does not terminate. |
HannesWell
force-pushed
the
add-norestart-option-to-launcher
branch
from
1551906 to
2ef1133
Compare
|
Just as a quick note: all the launcher binaries are now build in the Jenkins pipeline if anything changed. I'm not very familiar with the code itself, but I can try to have a look at this tomorrow. |
dhendriks
force-pushed
the
add-norestart-option-to-launcher
branch
4 times, most recently
from
56196e4 to
5071f51
Compare
There was a problem hiding this comment.
@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?
features/org.eclipse.equinox.executable.feature/library/eclipseCommon.h
Outdated
Show resolved
Hide resolved
features/org.eclipse.equinox.executable.feature/library/eclipseCommon.h
Outdated
Show resolved
Hide resolved
features/org.eclipse.equinox.executable.feature/library/eclipse.c
Outdated
Show resolved
Hide resolved
HannesWell
force-pushed
the
add-norestart-option-to-launcher
branch
from
e6a2a2a to
563232f
Compare
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. 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
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. |
- Fixed comment to match implementation. - Align comment/count lines to make it easier to keep them consistent. - Fixed source code indentation
HannesWell
force-pushed
the
add-norestart-option-to-launcher
branch
3 times, most recently
from
3334340 to
6db9c3c
Compare
|
Thank you for your detailed answer, I have checked also the surrounding code in more detail.
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
The reasons I find Why do you prefer just The doc gives the full list of supported options: Eventually we should update that as well.
Yes it looks like that. For example I cannot find a any definition nor a usage of the
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
force-pushed
the
add-norestart-option-to-launcher
branch
from
6db9c3c to
5c79e82
Compare
|
I changed |
dhendriks
changed the title
|
I updated the merge request title to account for now having |
Alright. Lets go with only @HannesWell Probably another build needs to be approved. If that succeeds, can we merge this? |
HannesWell
force-pushed
the
add-norestart-option-to-launcher
branch
from
5c79e82 to
14076d9
Compare
There was a problem hiding this comment.
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.noRestartthen.
Great.
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 theRofnoRestart. With that, it is now consistently--launcher.noRestart.
Yes, that makes sense. Thanks.
I have now squashed the commit that applies the suggestions from code review into your last commit and also adapted the commit message to the new name.
@HannesWell Probably another build needs to be approved. If that succeeds, can we merge this?
Yes, absolutely. I'm fine with this now and I assume we won't get more reviews from other committers, but I think it's a sane enhancements now.
Thanks for this enhancement and your patience. Also thanks to @umairsair for your help!
|
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 Which eventually will be part of |
Indeed, thanks @umairsair. And thanks to you as well, @HannesWell. And to the others that participated/helped. |
I created a pull request for it: eclipse-platform/eclipse.platform.releng.aggregator#2195 |
|
Thanks for your persistence and patience! 🏆 |
|
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? |
|
Ah, they seem to already be available, as Java property |
|
I've created a PR for the workbench restart not being available. See eclipse-platform/eclipse.platform.ui#2125. |
This adds a
-norestartoption to the launcher. When the option is used, it suppresses the restart behavior of exit codes 23 and 24, such that anIApplicationcan 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 extraifaround the body of twocases, with a comment at the end to explain the fall-through. The bodies of these two newifstatements contain the original unmodified original code of thecases, indented one extra level.See also the discussion at #378.