Community
Participate
Working Groups
Eclipse comes with a predefined eclipse.ini: -vmargs -Xms20m -Xmx256m If eclipse(.exe) is launched without any commandline argumente everything works as expected. However, if a user chooses to launch eclipse with a vm-argument, like this: eclipse.exe -vmargs -ea then all the vmargs in the ini file are ignored. Could be "feature" instead of "bug", but it feels buggy nontheless....
Could appending non name conflicting vm args and replacing conflicting args be an option, e.g. launching eclipse with "-Xmx512M -ea" should yield "-vmargs -Xms20m -Xmx512M -ea" for the default settings in eclipse.ini?
If someone step up to the task, everything is possible :). The problem is still that in some cases you want to completly override what is in the file.
(In reply to comment #2) One option could be to add a "-appendvmargs" switch and keep the current (replacing) behavior of "-vmargs".
There is some usecases. 1.) One does not specify any vm arguments on commandline Result: eclipse comes up with * start heap size: 40m * max heap size: 256m 2.) One does specify vm arguments that do not conflict, say: -ea Result: eclipse comes up with * assertions are enabled during the eclipse run * start heap size: java default (Larger of 1/64th of the machine's physical memory on the machine or some reasonable minimum. Before J2SE 5.0, the default initial heap size was a reasonable minimum, which varies by platform) * max heap size: Smaller of 1/4th of the physical memory or 1GB. Before J2SE 5.0, the default maximum heap size was 64MB 3.) One does specify vm arguments that do conflict, say: -ea -Xms10m * assertions are enabled during the eclipse run * start heap size: 10MB * max heap size: Smaller of 1/4th of the physical memory or 1GB. Before J2SE 5.0, the default maximum heap size was 64MB While 1 and 3 are pretty clear (conflicting args get overwritten), the second use case is the one in question. I'd say that for the user it's pretty confusing to specify one argument and thereby unspecify two others, significantly changing the behaviour of the vm. The settings in eclipse.ini should be considered integral part of the launch process and should only change if the user explicitely overwrites them...
Before embarking on what will eventually be a crazy story, I would like to understand why one specifies values on the command line and not in the <launcher>.ini file as it is recommended. To me, users that are willing to set various parameters represent a minority of and are considered to be advanced users and they should understand the implication of their changes. This has been the position we took when we implemeted this mechanism because we could not / had not the time, to figure out a nice solution. To me the difficult points in this story are: - how do we merge the same values (for example gc policies, heap sizes, vm specific args). And what happens if the two arguments that are specified collide and prevent the VM to start? - this last point brings the point of, how do I force an override to happen if I really want to. - Which is the same problem than providing backward compatibility. Also when proposing this type of behavior we then open the pendora box and now we will be asked to merge any type of arguments, etc... which will make our launcher even bigger than it is :). In any case the code is available from the platform-launcher CVS module.
(In reply to comment #5) > Before embarking on what will eventually be a crazy story, I would like to > understand why one specifies values on the command line and not in the > <launcher>.ini file as it is recommended. The -debug flag is a good example, we advise our customers to launch eclipse in "debug" mode to gather some input, along with that we want them to enable assertions. Many of them don't have write-access to config.ini, anyway, they would not want to make the change permanent or accidently modify the startup-parameters of someone sharing a network install. > - how do we merge the same values (for example gc policies, heap sizes, vm > specific args). I would say there should not be a merge of "-Xmx12m" and "-Xmx24m", the latter one wins > And what happens if the two arguments that are specified collide and prevent > the VM to start? like "-server -client"? java handles this by using the latter argument, there could be others, but the eclipse.ini has IMHO no difficult options anyway, the conflicts up to this date can only be -Xm related, and these options are handled by using the latter one by java, we could even route the eclipse.ini vm arguments through and append the user-specified ones, trusting that java will use the latter one, but that's no clean solution I would implement... > - this last point brings the point of, how do I force an override to happen if > I really want to. > - Which is the same problem than providing backward compatibility. I'd say an option "-ini <path-to-new-ini>" that also accepts "none" would be a good solution to this, it would also allow for a separate config.debug.ini that get's used upon "-debug" invocation and sets "-ea" and "-esa" or something....
Is it sufficient to have a -ini file that points to the eclipse.ini file to use (or none)? We would prefer not to have the launcher needing to parse vm arguments and trying to merge them.
*** Bug 162701 has been marked as a duplicate of this bug. ***
(In reply to comment #7) > Is it sufficient to have a -ini file that points to the eclipse.ini file to use > (or none)? > We would prefer not to have the launcher needing to parse vm arguments and > trying to merge them. I would suggest precedence instead of merging: command-line trumps eclipse.ini, simple as that. If conflicting parameters are generated, Eclipse doesn't start -- maybe puts out an error message noting the conflict so it can be resolved. I mentioned this problem as part of Bug 158823; the relevant part is: We have a root/user installation, and I'd like to put common parameters in the eclipse.ini in the root installation, but allow users to override those parameters on the command line. (As far as I can tell, I can't have a second eclipse.ini in the user installation anyway, at least when I launch from my user installation with the -install parameter pointing to my root installation). It's very important for us to run our application from read-only media (e.g. root installation or CD-ROM) and still allow some degree of customization. So far, an environment variable based launch is the only way I've found to do this effectively. I believe precedence-based variables would solve this problem without undue complexity.
I like the idea of the "-ini <ini-file>" option, this way one can merge the arguments him/herself by copying the existing ini and modifying it to the current needs. However, this is not a fix for the bug, namely the silent loss of vm arguments by using the -vmargs switch. IMHO it's better to append the users vm args to those of the .ini file than replacing them entirely. IF this leads to a command line java won't accept the launcher can print a little message hinting at the commandline java was given but as said before, many conflicting arguments can be given and the latter will silently be used, seemingly to support the appending approach ;)
An option --launcher.ini <inifile> has been added to the launcher.
*** Bug 218395 has been marked as a duplicate of this bug. ***
I have been troubled by this issue for long and only now even recognize this fact. My eclipse.ini needs some weird flags to work around (Sun)VM bugs: -XX:+UnlockDiagnosticVMOptions -XX:+UnsyncloadClass *additionally* (as I thought) I was setting different heap sizes depending on what projects I'm working on, plus more experiments with passing osgi properties like -Dosgi.support.class.certificate=false I had no idea that the original arguments from eclipse.ini were overridden, because I considered individual vm arguments as independent. Obviously, and looking at the launcher code (library/eclipseMain.c) vmargs is treated as one arg only, whereas I *interpreted* "-vmargs" as a switch between two arg-passing modes, where subsequent args were still interpreted individually. As a result I was blaming the VM for still causing deadlocks during class loading (which of course occurred at erratic intervals) despite the option which I passed in eclipse.ini. From this experience I have three suggestions: * let the launcher print a warning when vmargs are overridden from the command line * add another option -appendvmargs as suggested in comment 3 to support quick experimentation with arguments (before making changes persistent in the .ini). If passing this option causes an incompatible mix let the vm crash if you will, the user definitely is aware that (s)he is passing arguments from different sources. As a special bonus the launcher could print the resulting list of vm arguments. * Add the vmargs to the initial log message (the one starting with !SESSION). The reason I was barking up wrong trees for so long was: I had no clue what arguments the vm was actually working with. Hm, I was going to propose a simple addition to org.eclipse.core.runtime.adaptor.EclipseLog#writeSession() but I couldn't find a way to squeeze this information out of a running vm, so maybe this part isn't easily possible, currently. PS: another use case is launching runtime workbenches which should share the vmargs of the host's eclipse.ini plus perhaps application specific vmargs via the launch configuration. Which piece of software is merging those bits? Obviously eclipseMain.c is not involved here?
Hi Andrew :) Is there anything I can do to help get this resolved in 3.5.1, maybe provide a patch or something? This small issue represents a show stopper for us, we need to override the location of the vm and have no means to do now, I would have to modify the plug-in locally myself, which is nasty. I'd rather use your fix in this area... Helmut
Helmut, I don't think this would get approved at this point for 3.5.1. However, the location of the vm is not a vm argument, it is a program arg. Program args from the command line are added to and over-ride the arguments from the launcher.ini. I don't see how this bug stops you from changing the vm.
Ciao Andrew :) That's right, -vm ../jre are program arguments. And yes, even though I add them as arguments to the product they never find their way into the .ini file because of this very bug ;) You see how I can't use RCPs that way? HTH, Ciao, hh
Helmut, now I'm just confused. Maybe you've got the wrong bug? This is about the native launcher using the command line vs the eclipse.ini. This > I add them as arguments to the product they never find their way into the .ini > file sounds like you are talking about the .product file and creating the .ini file.
Andrew, you're right, I was talking about 282303, please forget about comments 14 and 16, i'll ask for your help in that other bug then! :)
Another use-case which I couldn't see here is using -vmargs to supply values that are expanded by the shell. For example, to workaround the Windows user.home bug [1], one can run Eclipse using: eclipse.exe -vmargs -Duser.home=%USERPROFILE% This cannot be moved into eclipse.ini since environment variables cannot be referenced. So due to this bug all other vmargs are discarded when the above is used. [1] http://bugs.sun.com/view_bug.do?bug_id=4787931
I would like to bring this issue on the table for considering in 3.6. This is a major (if not critical) issue quite nicely described in comment 13: As a user putting a -vmargs option on the command-line starting Eclipse, I do not expect the application-defined vmargs from the launcher.ini file to be silently ignored as a side-effect. Also, as a product builder, I want to configure my product with all relevant suitable options in the launcher.ini file. I know what vm is being shipped and what options are suitable. I don't want these options to be overridden accidentally. Just take the Eclipse packages for instance, which configure a -Xmx setting that's suitable for the product. A user can very easily override this accidentally, with the effect that the application may crash due to OutOfMemoryError, which may lead to loss of data -- that's why I consider this issue critical. From all the solutions proposed on this bug, I think the following acceptance criteria need to apply: 1. It must be possible to specify vmargs in the launcher.ini which cannot be overridden accidentally. 2. It must be possible to use commandline vmargs which are appended to the launcher vmargs - that's the simplest and most needed precedence scenario, which I expect to work in 99% of the cases. 3. Product builders must be able to write a launcher.ini in a way that works the same as the current launcher.ini (avoid regression). Additional should-have: 4. It should be possible to write a commandline which explicitly overrides vmargs from the launcher.ini (workaround: specify a different .ini file on commandline, this is possible thanks to comment 11. And here is my concrete implementation suggestion: * Add a new "--launcher.stickyVmargs" option which changes the behavior of the Launcher such that commandline vmargs do not override launcher vmargs but append to them. I expect this option to be seen in most launcher.ini files, in order to fix this issue. * (Optional) add a new "--launcher.overrideVmargs" option which restores current vmargs behavior. I expect this option to be seen rarely on a commandline when launcher.ini args need to be deliberately overridden. This option is not strictly needed because the -ini option from comment 11 can be used as a workaround.
Created attachment 167132 [details] Patch v1 implementing --launcher.stickyVmargs Attached patch implements --launcher.stickyVmargs. Developed and tested on Linux-GTK, I could not see any dependencies onto other OS/WS so I think that this should be fine. The patch is really simple, and if we want --launcher.overrideVmargs too that's super simple to add. Can this be considered for 3.6RC1 ?
Adding polish kwd as discussed on http://wiki.eclipse.org/Eclipse/PMC
(In reply to comment #21) > Created an attachment (id=167132) [details] > Patch v1 implementing --launcher.stickyVmargs > > Attached patch implements --launcher.stickyVmargs. Great, I appreciate! Quick question to any p2-experts: if the patch is adopted will the following work in p2.inf: instructions.install = \ addProgramArg(programArg:--launcher.stickyVmargs); \ addProgramArg(programArg:-Xmx512m); Until now I'm using addJvmArg which is specialized for -vmargs, so can addProgramArg be used for the new option out of the box?
(In reply to comment #23) I'm not a p2 expert nor do I know the p2.inf or "addProgramArg" syntax, but from a high level I assume that this won't work because the -Xmx512m argument must be placed after a -vmargs argument to work. In other words, --launcher.stickyVmargs is a "switch" which changes behavior of the existing -vmargs switch. It doesn't allow you to mix vmargs with normal program args. I was originally considering an alternative approach where you would write e.g. --launcher.vmarg -Xmx512m --launcher.vmarg -Xverify:none i.e. have ONE special switch which takes the next argument and adds it to the list of sticky vmargs. I assume that this approach would address your request. Can you explain the rationale behind your request, i.e. what kind of problems would it solve for you if p2.inf supported the syntax you proposed?
The RT PMC has already -1'd new feature changes in M7 (bug 102239). We are now partway though RC1
I see your point, Andrew, but this issue is IMO not a new feature but a fix for a long-standing serious usability problem which can have critical impact. Today, the Launcher just doesn't behave as intuitively expected, so I consider this a bug and not a feature. Keeping the "3.6 or not" question out of the game for a moment, can you assess the quality of the patch and let me know if you see any technical problems?
It is semantics, but the current replacement behaviour was by design which is why I would call this a featue instead of a bug. I will look at the patch today.
(In reply to comment #24) > In other words, --launcher.stickyVmargs is a "switch" which changes behavior of > the existing -vmargs switch. It doesn't allow you to mix vmargs with normal > program args. Thanks for clarification. > I was originally considering an alternative approach where you would write e.g. [...] I wouldn't want to open up the design behind it, for me getting this soon has priority over syntactical details. > Can you explain the rationale behind your request, i.e. what kind of problems > would it solve for you if p2.inf supported the syntax you proposed? I just want to be sure that a feature installed using p2 can add its vmargs in a way that prevents accidental overriding from the command line. From what you say it seems I'd be doing this: instructions.install = \ addProgramArg(programArg:--launcher.stickyVmargs); \ addJvmArg(jvmArg:-Xmx512m); (with -Xmx just serving as a placeholder for more interesting args). I'd be fine with that.
(In reply to comment #28) > instructions.install = \ > addProgramArg(programArg:--launcher.stickyVmargs); \ > addJvmArg(jvmArg:-Xmx512m); Yes, assuming that these instructions in p2.inf lead to creating a launcher.ini file with the respective arguments, I think this should work. (In reply to comment #27) > It is semantics, but the current replacement behaviour was by design which is > why I would call this a featue instead of a bug. Well I don't want to start nitpicking here, but to me it looks like the design was flawed since it's too easy to accidentally remove essential vmargs such as -Xmx or the ones mentioned in comment 13, which are vital for the application that comes with launcher.ini to function. End users who just see the interface (eclipse.exe, or myrcp.exe) and don't know the implementation details (that vmargs are coming from launcher.ini) perceive it as a bug in the appliation when it starts crashing just because of adding an innocent -vmargs -DMYFLAG. I don't buy Pascal's argument from comment 5, that people who set -vmargs are advanced enough to know what they are doing and thus intuitively replicate all launcher.ini vmargs on their commandline. Quite frankly, I'd like to see the new --launcher.stickyVmargs semantics be the default one, thus fixing (what I call a bug). But I do see that it's late in the game, so my proposal is not changing current semantics. We could discuss an alternative implementation, that gives the author of launcher.ini more flexibility to specify both sticky vmargs and non-sticky ones. At the moment, I consider this overkill, but I could contribute a solution along these lines too if it's preferred. And BTW, I think there's another flaw in the current semantics, since if you consistently want to override launcher.ini vmargs to be on the safe side, then setting -vm from the outside should also override any launcher.ini vmargs. A new --launcher.overrideVmargs switch could fix this easily, such that running an RCP which comes with an IBM JVM and IBM specific vmargs in its launcher.ini can be launched with a Sun JVM on commandline. I think the main argument here is that append-vmargs trumps override-vmargs, because any error that would be caused by accidental appending would be obvious (by the JVM not starting or issuing an error), whereas accidental overriding causes the application to start but run into late, unexpected and hard-to-diagnose errors (as comment 13 shows). Thanks in advance for reviewing the patch.
No matter what your view is on the current semantics (flawed or by design) this patch clearly falls under the category of an enhancement because you are adding new behavior along with a new launcher option. There is no way around that. This will need PMC approval IMO and Andrew is correct in asking for it. This is awfully late in the game and I fear we will have a hard time making sure all the details are covered correctly. It will depend on Andrew's assessment on the risk of the change and if that outweighs the need to add this enhancement.
Ok, I agree that it's late (too bad that we needed this for our product only recently). I think that I've clearly outlined the end-user as well as product builder's problems with the current behavior, and also the value of the patch. Let's wait for Andrew's assessment of risk.
Created attachment 167244 [details] Patch v2 The patch itself is relatively simple. The risk here is mostly associated with the fact that most of the changes are in the executable itself instead of in the shared library. (Windows users generally can't update to new versions of the executable because it is locked by the running process). There is the question of what the vm does if the user duplicates arguments that are already in the ini file, some quick testing indicates that the second arg wins, at least for -D args. Note that on the mac, we ourselves will process the vm args looking for -Xdock:icon and -Xdock:name, we stop the processing once we find both the icon and the name, but if one is repeated before the other is specified, then the second instance wins. The --launcher.stickyVmargs does not need to be in the .ini file, it can be specified on the command line. If it is given in the .ini file, the user has no way to override the vm arguments without editing the file. I made some minor changes to the patch, in concatArgs, our win32 compiler wants all the local variables defined first. Also, I changed the way we count the arguments. The call to concatArgs was moved up to near where we handle the users program arguments.
Thanks for the quick review, Andrew! I had thought about he EXE vs sharedlib issue myself. Moving the code for vmarg processing into the sharedlib shouldn't actually be too hard, but it looks like it would need a new sharedlib entry point ... /** returns combined config and user vmargs as per vmargsPolicy, * and removes removes vmargs from the input lists. */ _TCHAR** getVmargs(&configArgs, &userArgs) would that be considered an API addition or not since the sharedlib is considered "internal"? I got an impression that the handling of vmargs from a .ee file is already done in the sharedlib, so this shows that it's possible. Regarding the switch, what about changing to --launcher.vmargsPolicy append --launcher.vmargsPolicy override which could be handled by the sharedlib as well, would provide the symmetry between launcher.ini and commandline that you requested, and would provide room for future extension of the mechanism in case anyone needs it. I can work on a patch along these lines if it's desired, but I think I'd first hear an RT PMC's opinion about it.
Created attachment 167258 [details] Patch v3 implementing --launcher.vmargsPolicy Attached patch moves the vmargs processing into the Launcher sharedlib, as suggested by Andrew, and changes the option such that explicit overriding is possible from commandline, e.g. eclipse -vm myvm.ee --launcher.vmargsPolicy override -vmargs -DFOO=bar Developed and tested on Linux GTK. I really like this approach because it satisfies all acceptance criteria, and it's flexible for future improvements in the launcher sharedlib. The additional sharedlib entrypoint is backward compatible with older launcher exe's. Andrew, can you give this another review?
(In reply to comment #33) > I can work on a patch along these lines if it's desired, but I think I'd first > hear an RT PMC's opinion about it. I don't think the additional entry point into the shared library is considered API. This is an internal interface between the exe and the shared library. I don't think we have ever considered that API. But we are still dealing with an API type with the new launcher arguments. I consider the launcher arguments to be API since we document what the options are and users come to depend on the options. (In reply to comment #34) > Created an attachment (id=167258) [details] > Patch v3 implementing --launcher.vmargsPolicy > > Attached patch moves the vmargs processing into the Launcher sharedlib, as > suggested by Andrew, and changes the option such that explicit overriding is > possible from commandline, e.g. > > eclipse -vm myvm.ee --launcher.vmargsPolicy override -vmargs -DFOO=bar > > Developed and tested on Linux GTK. I really like this approach because it > satisfies all acceptance criteria, and it's flexible for future improvements in > the launcher sharedlib. The additional sharedlib entrypoint is backward > compatible with older launcher exe's. > I have not looked at the patch, but is the only new option now --launcher.vmargsPolicy? What is the default policy? - The default must be override, like it is today. - If the vmargsPolicy option is specified on the commandline then the vmargsPolicy option (if specified) must be ignored from the eclipse.ini.
(In reply to comment #35) Yes, --launcher.vmargsPolicy is now the only new option. Yes, "override" is the default behavior. Yes, if --launcher.vmargsPolicy somearg is specified on the commandline, then any different vmargsPolicy from the ini file is ignored. Specifying --launcher.vmargsPolicy without an argument is accepted by the Launcher i.e. doesn't lead to errors, but it has no effect. In other words, if you specify --launcher.vmargsPolicy append in the Launcher.ini file and --launcher.vmargsPolicy (nothing) on commandline, the launcher.ini file wins since you didn't specify on commandline what policy you want. Missing or invalid arguments to --launcher.vmargsPolicy are currently not reported as an error - I could add that if desired. I can also add documentation into the file comment header as well as the runtime-options developer docs http://help.eclipse.org/galileo/index.jsp?topic=/org.eclipse.platform.doc.isv/reference/misc/runtime-options.html I can do that once there is agreement on the feature. And by the way, a --help option for the launcher which prints all accepted arguments would be fantastic -- but that's a different story and non-trivial since arguments can be accepted on all layers of Eclipse (well perhaps --launcher.help would make sense).
On second thought, going back to TWO options --launcher.appendVmargs --launcher.overrideVmargs might be better, because 1. These are easier to type and to remember 2. Without an argument, there is no problem of error reporting / invalid args 3. The launcher isn't really extendable / pluggable for other policies anyways I can make patch v4 along these lines if desired.
Created attachment 167367 [details] v4 I do like this better in the shared library. The new shared library will be backward compatible with old exes, however the reverse is not true. Having a new exe with an old library is not a common case, but we may want to be more forgiving here. The patch fails in this case, we can instead just carry on with the old behaviour. I worry a little about proliferating "interface" methods between the exe and the library. Instead of a new "setConfigVmargs" I made a "setAdditionalArgs" which takes a structure. In the future the structure can be expanded with additional fields if we want to pass other information into the shared library. I moved the "concatArgs" method into eclipseUtil, in 3.7 we should review the code for places where we are joining arrays and use this utility method instead, I raised bug 311948. I don't think I like the policy argument being optional, specifying just a bare "--launcher.vmargsPolicy" doesn't affect anything. That said, there is a general problem in the case of missing non-optional values, I raised bug 311947. I have left the optionality there for now, but I don't think this should be documented as optional.
Created attachment 167368 [details] project settings Changes to project settings. just keeping them separate from the actual code changes under review
I'm still not convinced this is a must fix enhancement at this stage in the release. Are you suggesting this is a blocking bug? It has been around for a long time. I really think we should defer this to the next release.
I agree that we should defer fixing for 3.6. It is too late and changing launcher behaviour is both subtle and may cause changes in behaviour expected by others. Doing that late in the game is not a great idea.
With the addition of the s390(x) platforms, compiling the launcher now has manual steps that require jumping through a number of hoops. I do not like the idea of having to do this very many times close to RCx deadlines. I would suggest that if this isn't in by tomorrow then it should slip to 3.7
(In reply to comment #38) Hi Andrew, thanks again for the review. I agree that all your changes make sense. Especially thanks for filing the followup "cleanup" bugs. > The new shared library will be backward compatible with old exes, however the > reverse is not true. Having a new exe with an old library is not a common That's true. If we are really concerned about this case, we could treat the case of symbol-not-found with a warning only rather than an error. I can easily do this if you want. > the library. Instead of a new "setConfigVmargs" I made a "setAdditionalArgs" > which takes a structure. In the future the structure can be expanded with > additional fields if we want to pass other information into the shared Hm... I'm not sure if I understand you right. If you want the structure to be extendable in the future, wouldn't you need to define a fixed "version" field in the structure, such that the consumer knows what version of the struct the producer has passed in? Otherwise you'd end up at risk reading uninitialized memory when the exe is old but the sharedlib expects a struct with more fields. I'm also not sure what kind of additional args you'd expect in the future. In principle, the sharedlib knows everything that the exe knows already today (because it gets the initial args). So we could just go and re-implement the inifile reading logic in the sharedlib -- and thus not only avoid any additional method but also be able to live with any older exe version. I just wanted to avoid this because I disliked the performance penalty of reading the same file twice... but multi-version-robustness may trump this argument. Basically all that the exe needs to do, is find the sharedlib -- anything else should ideally live in the sharedlib. Given that we want to move more and more functionality from the exe into the sharedlib moving forward, I'm not sure whether we'll really need extendable args in the future. Passing in initial args and the inifile contents should always suffice. For 3.6 we're already modifying the exe anyways (because it got the new --launcher.defaultAction) so I don't think we need to be especially careful about what we're putting into the exe or sharedlib right now. But if we think it's a better approach, I can easily come up with a patch that just re-reads the inifile in the sharedlib and thus avoids the new sharedlib entrypoint. > I don't think I like the policy argument being optional, specifying just > a bare "--launcher.vmargsPolicy" doesn't affect anything. True, the arg isn't optional, an errormessage in case of a missing arg (or arg not in the allowed range of args) would be better. I'll follow up separately why I think this is important for 3.6.
(In reply to comment #43) > Hm... I'm not sure if I understand you right. If you want the structure to be > extendable in the future, wouldn't you need to define a fixed "version" field > in the structure, such that the consumer knows what version of the struct the > producer has passed in? Otherwise you'd end up at risk reading uninitialized > memory when the exe is old but the sharedlib expects a struct with more fields. I guess my original idea assumed the exe and shared library were in synch. You are right that if they are out of synch with each other we need some kind of version field at the beginning of the structure. > For 3.6 we're already modifying the exe anyways (because it got the new > --launcher.defaultAction) so I don't think we need to be especially careful > about what we're putting into the exe or sharedlib right now. While the exe has changed a little in 3.6, the 3.5.x exe still works, the defaultAction stuff did not affect the exe.
Tom and Jeff - I agree that it is very late, and the launcher is not a beast that's particularly easy to iterate on late in the game. I also need to admit that we can work around this in our commercial product. We've been using a separate wrapper exe to pass in our args up until now; and I re-discovered this issue when we tried getting rid of this wrapper exe in favor of the generic Launcher. So our workaround is keeping our wrapper exe. On the other hand, here are my arguments why this is important for 3.6: 1. Usability: Unexpected errors that are hard to debug. As a user passing in a vmarg on commandline, I surely do not expect that this may have side-effects (by disabling vmargs in the launcher.ini). That's a fact which no documentation can work around. 2. Severity: Unexpectedly overriding an essential vmarg such as -Xmx can easily lead to severe errors (OutOfMemoryError, data loss) or product hang-up (as explained in comment 13). This threat is real: Bugzilla is full of OutOfMemoryError reports with EPP packages, all of which define their -Xmx in the launcher.ini file. So I don't think it is a blocker, but it is critical. 3. No workaround in Open Source: As a product builder, I don't see any alternative for placing a -Xmx argument that's required by my application anywhere else but in the launcher.ini file. Putting it into the .ee file is not a good option since .ee vmargs are always appended last and thus cannot be overridden on commandline; also, .ee files do not work when my product doesn't ship a VM. Shipping a batchfile, shellscript or wrapper exe along with my app doesn't blend well with RCP's that should integrate into a desktop UI. As I have mentioned before, I consider this issue more as a bug (counter-intuitive functionality) than a new feature. My personal favorite would be making the vmargs "append" semantics the default, because that provides better error reporting, more flexibility, and consistency with application args. I do see that this is not an option due to backward compatibility requirements, but quite frankly I envision that moving forward all newly written .ini files will have the "--launcher.appendVmargs" option and that users will never think about using the "--launcher.overrideVmargs" on commandline. Now that all being said, what is the real risk of putting this in? The patch is small, and there's not so much new functionality. I think I could even come up with a Unittest to run through all cases if it helps. IMO the biggest risk is sub-optimal naming of the new options but there's not so much choice after all. The second risk is unexpected compilation problems on esoteric platforms, and I agree with Andrew's deadline (tomorrow) to address this. So - I'm not going to fight for this (since we don't depend on it commercially), but for the Eclipse Community at large, I believe that this should go into 3.6. It's my personal polish item if you will. Jeff and Tom, please consider the points I have made and I'll accept your decision.
(In reply to comment #44) > While the exe has changed a little in 3.6, the 3.5.x exe still works, the > defaultAction stuff did not affect the exe. Ok... if compatibility with the 3.5.x exe is a requirement, I think that would be doable simply by re-reading the inifile in the sharedlib. Given that it's in disk cache, performance penalty should be minimal. Plus, re-reading the inifile would only be nexessary if "-vmargs" appears on the "initialArgs" list. Then even a 3.5.x exe could work with the 3.6 sharedlib and a 3.6 inifile format.
Given the amount of discussion that is still going on, I am against pushing this into 3.6.0. Definitely see the need in 3.7, thanks Martin for working a fix! Perhaps this is something we can consider for 3.6.1?
3.6.1 would work for me, but would it work for you with new launcher args being defined?
(In reply to comment #48) > 3.6.1 would work for me, but would it work for you with new launcher args being > defined? I think it would give us the necessary time to make sure the options are correct. As far as adding them in 3.6.1, it seems less of a risk than adding them at this point in the release. We would have to make sure to add them early in July to get some more confidence.
Fine with me. Let's set a 3.6.1 target milestone, and I'll work on a patch that avoids introducing a new DLL entrypoint in order to be able and work with old launcher Exe's -- in a Service Release like 3.6.1 I believe this is a must-have since we need to expect that users will update from 3.6 to 3.6.1 without exchanging their Exe.
I'm thinking that WindRiver has the skill and interest in becoming a launcher committer...
Created attachment 167413 [details] Patch v5 --launcher.appendVmargs Attached patch implements two new arguments (--launcher.appendVmargs and --launcher.overrideVmargs), without adding a sharedlib entrypoint. Doing this was actually surprisingly simple, since much functionality can be re-used through eclipseConfig.[ch]. This variant is my absolute favorite so far :)
Created attachment 167418 [details] Proposed ISV docs for patch v5 Attached is the proposed documentation in o.e.doc.isv/refernece/runtime-options: --launcher.appendVmargs (Executable) If specified, any VM arguments on the commandline will be appended to any VM arguments specified in the launcher .ini file. Using this option is recommended in every launcher .ini file that specifies VM arguments, because the default behavior of overriding VM arguments can have unexpected side-effects. --launcher.overrideVmargs (Executable) If specified on the commandline, overrides the effect of --launcher.appendVmargs in a launcher .ini file such that none of the VM arguments in the .ini file are considered as soon as a -vmargs option is detected on the commandline. I'm marking patch v4 obsolete since it does not satisfy the "works with old exe" criterion. I believe that patch v5 could be the final solution, unless there are strong objections against having two switches, since it satisfies all criteria. I'm attaching the proposed user docs to allow commenters judge how the two separate options feel.
Moving to 3.6.1 for consideration.
Created attachment 172145 [details] v6 New patch. Changes to eclipseConfig were moved to bug 316975. The real change here is to do with the processVMArgs function, which performs platform specific processing on the vm args being used. (Currently mac is the only platform which does something here). This processing should happen on the actual vm args that are going to be used. So we must adjustVMArgs first.
Ping, how can we move forward on this? On a high level, current status is: - No change in behavior unless eclipse.ini is changed - Can add --launcher.appendVmargs into eclipse.ini if desired by product - Can add --launcher.overrideVmargs on commandline to force override - Patched DLL works with old launcher exe (though new launcher exe is slightly more efficient) I'd still love to see this in 3.6.1 since it would enable our commercial product get rid of our home-grown wrapper around the original Equinox Launcher. For us it's a must-have that we can equip the product with a special -Xmx vmarg but still allow end users to easily override (increase) their own -Xmx vmarg.
I will review this with Andrew when he gets back tomorrow.
Created attachment 176334 [details] v7 (In reply to comment #55) I carefully reviewed the differences between patch v5 and patch v6. Other than whitespace and trivial changes, the only relevant change is the ordering of how VM arguments are processed. Doing the platform-specific processVMArgs() last makes sense. But in the case no user-defined VM Args exist (from cmdline or launcher ini), there is now a change in behavior: - with v5 and before, adjustVMArgs() would potentially add a XX:MaxPermSize to the empty arglist - with v6, adjustVMArgs() would not be called, and therefore the XX:MaxPermSize would not be applied in this case. To resolve this, I think you should put the adjustVMArgs() back into the getVMCommand() function, and put the processVMArgs() right behind it; I see now reason why the processVMArgs() should be applied to user-VMargs only and not also the window system specific default-vmargs in case there are no user-VMargs. This is a small change in functionality too, but IMO a less problematic one. Attached patch v7 has only this changed compared to your v6.
Created attachment 176336 [details] v8 If you are still concerned about the slight change in functionality mentioned above, the only solution is taking the code around /* Prepend Launcher Vmargs if policy append was requested */ out of the adjustVMArgs() function and put it into a function of its own - call it mergeUserVMArgs(). Attached patch v8 does this; it also brings the code slightly closer to the original code we have in HEAD right now. Small drawback of this approach is that we cannot easily detect whether we need to free any memory used for merged VM Args (the "argsAllocated" variable from v7 and before), so this does introduce a small memory leak.
Created attachment 176387 [details] v9 I don't have any problem with the change in behaviour described for v7 since in practice there is currently no overlap between the default vm args and the platform specific processing. However, given that this is proposed for 3.6.1, I think it is important to modify as little as possible. As well, I like the separation of having the merge code in its own function. So I think we should go with v8. Here is patch v9. It has a change to mergeUserVMArgs so that it always returns new memory, this relies on concatArgs handling NULL as one of its arguments. We then free the memory at the end of the run method where we do other cleanup. Martin, if you don't see any problems with these changes, I will look at putting this in HEAD this week so we can try it out a bit. On a side note, for whitespace, I've started using CDT's source formatting by copying individual functions into a temporary .c file. I'm doing this on selected blocks as they get modified for other reasons. I've left some of the formatting out of this patch for clarity, but will apply the formatting to HEAD.
+1 v9 looks good to me! I compared against the old v5 as well as current HEAD and couldn't find any flaws except a comment typo ("This always allocate new memory"). Didn't compile and test it though. Moving forward, I see some potential for further optimization, but this won't affect the backport and I can file separate defects to work on these: - The launcher.exe could call setConfigArgs() in the launcher.dll such that the DLL doesn't need to re-read the ini file unnecessarily - In adjustVMArgs(), the search for XXPERMGEN could use the new indexOf() - Unittests should be added for the launcher exe - Comment in eclipseMain.c line 143 is incorrect - see createUserArgs() Thanks!
I had released the source to HEAD and was just checking the compilation on windows when I noticed a few problems there (I've been working on linux, so did not notice this earlier). 1) bug 322594, though it does not occur normally, there are cases where the initialArgv may not be properly null terminated. There is room at the end of the array for a null, but that location remains uninitialized leading to a crash in mergeVmargs. I think here we should force null termination in setInitialArgs. This will probably be covered under bug 322594. 2) On windows, when running eclipsec.exe, we do not append the arguments from the .ini file because we fail to read "eclipsec.ini". The code that falls back to reading "eclipse.ini" instead is only there in the eclipsec exe and is ifdef'd out for the shared library. There is a "consoleLauncher" static that indicates whether or not we are usin eclipsec. At this point, patch v9 is in HEAD but nothing has been compiled.
(In reply to comment #62) Uh-oh... looks like it's really time for some unittests! I just contributed some on bug 312059, hoping that these are valuable closing this bug. > 1) bug 322594, initialArgv may not be properly null terminated. Yuck, that's bad! When working on a fix, remember that the new DLL must be able to work with an old exe... putting the fix into setInitialArgs sounds like a good approach, since in that place we have both initialArgc and initialArgv so we can fix it easily. > 2) On windows, when running eclipsec.exe, we do not append the arguments from > the .ini file because we fail to read "eclipsec.ini". Hm. Just duplicating the fallback code seems like the most obvious fix. For the case where we have a new EXE, as mentioned before, I suggest adding a setIniArgs() method to the DLL such that the INI is only read once.
Created attachment 176582 [details] v10 The NULL termination of the initialArgv only seems to be wrong when using the cygwin make file, which I do for testing/debugging on windows. We use a microsoft compiler for the binary that we ship and I believe that this is not a problem there. So I don't think we need to worry about this. There are a bit more details in bug 322594. Here is a new patch: - getConfigArgs does not need to rescan the argument list looking for the ini file, we've already scanned the user args, we just need to store the ini file location. - I've added a new getIniFile(_TCHAR* program, int consoleLauncher) to eclipseConfig.c. This does modify both the executable and the shared library, but the new shared library still works with the old executable and vice versa. Code in the shared library should use getIniFile & readConfigFile. - --launcher.appendVmargs or --launcher.overrideVmargs specified on the command line does not survive a switch workspace operation. So we need to pass this in the generated vm command. Also, because some rcp apps are sensitive to unexpected arguments, we also need to consume the arguments in java Main. The patch now affects all three of the executable, library and launcher.jar. With just the shared library, you get the new behaviour, but your app receives a new command line arg. The launcher jar consumes that arg. The exe is not needed, it only changed because it shares some source code with the library.
Created attachment 177261 [details] latest Latest patch with changes from bug 323422
(In reply to comment #65) > Created an attachment (id=177261) [details] > latest > > Latest patch with changes from bug 323422 This last find should really make us nervous about putting this into 3.6.1. This is the reason our tests were not running properly during the nightly builds. We really need more time to test this out in Indigo before considering backporting to 3.6.x.
Created attachment 177356 [details] latest
Ping. If we don't want to end up with the same result for 3.6.2 as described in comment 66 we should either put this in now or defer to 3.7.
This is already fixed in 3.7 under bug323533 My vote is not fix this in 3.6.2 because of the risks that we have observed with making such enhancements.
(In reply to comment #69) > This is already fixed in 3.7 under bug323533 > > My vote is not fix this in 3.6.2 because of the risks that we have observed > with making such enhancements. +1.
We decided against 3.6.2. Marking as fixed in 3.7 M2.
I just tested this with Eclipse 3.7.1 (Build id: 20110916-0149) and it appears it's not fixed. What happened to this fix? Or am I somehow mistaken? This issue is a real big obstacle for me due to the issue stated in comment 19.
False alarm. I tested it wrong! Didn't have the --launcher.appendVmargs argument like I was supposed to. Sorry!
