Community
Participate
Working Groups
Build ID: I20081030-1917 Running on Windows XP 64 Steps To Reproduce: 1. Create any kind of ant script (use attached for example) 2. Right-click on it, go to External Tools Configurations, create new 3. In the Arguments section click on Variables and select file_prompt 4. Click apply - you will be prompted for file name. This is a bug, it shouldn't ask for a file name, what for??? 4. Save your configuration. 5. Right-click on your ant script and select Run as -> Ant Build... Notice that you get prompted for a file name, then dialog appears. 6. In the dialog click Run and you will be prompted for a file name - again? So it shows file prompt twice - this is a BUG. 7. Now, right-click on your ant script and go to Run As -> External Tools Configurations. Again it shows a file selection dialog - why? Bug too. Looks like it is all related to usage of $file_prompt in the run configuration - it pops up this dialog when it isn't even running your script. More information: This bug was originally observed in Rational Application Developer 7.5 (which uses Eclipse 3.4 runtime). Verified on Eclipse 3.5 M3 and bug is still there.
Created attachment 119057 [details] Sample ant task
Customer reported this issue through PMR 38704,047,649. There actually are side effects. Later, when you try to edit the launch configuration, it will prompt you for a file as soon as you focus on the Launch Configuration. If you happen to hit "escape" at that point, your launch configuration will get corrupted (all targets will be unselected). Please fix this issue if possible. Thanks.
Created attachment 143496 [details] Patch_1.0 User was prompted for a filename whenever variable in configuration was being resolved. Usage of ExternalToolUtil#getArguments always causes variables resolving and should be used only while running script, not in case of opening external tool dialog or performing apply after changes in configuration. I removed variables resolving in methods that are not used while launching script.
Darin, The patch and the diagnosis looks fine. Could you please review the patch by yourself and release it? The diagnosis is in comment 3, business impact in comment 2.
The attached patch does avoid the prompts, but could have other side effects. For example, variables that do not prompt will no longer be resolved when computing arguments during non-launch processing. I wonder if we can do better and just avoid the *_prompt variables when not launching.
Darin, I do understand your concerns, but what is the point of resolving any variables before launching? Is there anything that we should check? Actually it is possible to resolve only non-prompting variables but: (1) it leads to inconsistent state (some variables are resolved, some are not) (2) it requires API change Other launch configuration resolve variables on launch time.
Perhaps Darin(S) can comment here. My concern was effecting the context the Ant model uses for target resolution (i.e. by not resolving variables, does it potentially change the Ant model in an important way?). If not, then avoiding variable resolution until launch is probably fine. Darin (S)?
By changing to not resolve any Ant properties you can potentially lose the Ant model's ability to resolve some of the targets. For example: <import file="${importFileName}" /> None of the targets from the imported build file could be resolved. This has been an unfortunate long standing bug with the current implementation. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=175676 for more details and pointer to https://bugs.eclipse.org/bugs/show_bug.cgi?id=76226 which contains test cases for target resolution based on property resolution. The past argument was that with the relatively low usage of string_prompt or file_prompt in production buildfiles this bug was deemed not worth the investment to fix.
Darin(S), This problem is not only about annoying prompts. If user clicks "Cancel" at that point (because he is not launching script), launch configuration will get corrupted and all targets will dissapear form Targets tab. And that is really confusing.
...and what about not resolving just *_prompt variables when not launching?
Created attachment 145431 [details] AntPrompts_v.1.0 There is simple fix to avoid corruption of launch configuration after user chooses Cancel on prompt dialog. I added try { model.setProperties(getAllProperties(config)); } catch (CoreException ex){} The same is done in AntModel#setExtraProperties and AntJRETab#applySeparateVMAttributes. Looking on AntJRETab#applySeparateVMAttributes I am wondering if resolving variables is really needed here? This method is only used to set the logger - we are searching for "-logger" parameter. Is it possible that one of resolved variables will have such value? Can such value be given via prompt dialog? If we will remove resolving of prompt variables from this method, then we can avoid prompting while "Apply" and while switching to JRE tab. And that will be much improvement for the client!
Created attachment 145797 [details] patch This patch (1) uses the suggestion from comment#11 to avoid resolution when looking for the existence of the "-logger" argument. (2) avoids resolution of *_prompt variables when processing -D args for target resolution
Natalia, can you verify if this patch meets your needs?
Created attachment 145798 [details] patch with copyright updates updated copyrights to 2009
Created attachment 145906 [details] AntPrompts_v.2.0 (In reply to comment #13) > Natalia, can you verify if this patch meets your needs? > Yes Darin, this patch works as is needed. However I believe that we should also avoid resolving *_prompt variables in AntModel#setExtraProperties. This method process changes on Properties tab in External Tool Dialog. I include this change in this patch. Now Ant External Tool Dialog will work correctly with prompts. The prompt variables are resolved correctly during launch process. I am waiting for this patch to be commited.
Applied patch. Please verify, Mike.
looks fine, although the fix will cause anyone who provides a variable with _prompt in it to no longer be evaluated - in the off chance that someone does name their variable *_prompt* and it does not prompt...
(In reply to comment #17) > looks fine, although the fix will cause anyone who provides a variable with > _prompt in it to no longer be evaluated - in the off chance that someone does > name their variable *_prompt* and it does not prompt... Michael, Why do we need *_propmt variable to prompt if we are not launching script but just changing launch config? All prompts will appear during launch.
(In reply to comment #18) > Why do we need *_propmt variable to prompt if we are not launching script but > just changing launch config? All prompts will appear during launch. Mike is just pointing out that we are using the '*_prompt' pattern as a heuristic for identifying prompting variables. There's nothing that prevents someone from contirbuting a variable with this sort of name that does not actually prompt. We don't know of any such variables, it's just a note.
Re-opening to mark as 3.5.1 candidate.
Adding Dani for PMC approval.
Looked at the patch. The whole story is still pretty scary not to say broken but the patch fixes the most common issues. There's only one change that I'd like to see changed: there's a small risk that someone contributes a variable that resolves to "-logger". This is now doomed because the changes to AntJRETab don't use the string pattern like its done at the other places. +1 for 3.5.1 if the above mentioned change is made.
Created attachment 146224 [details] patch Additional patch for HEAD that uses pattern matching when searching for the -logger argument.
Created attachment 146227 [details] combined patch for 3.5.1
Fixed in HEAD and 3.5.x. Please verify again, Mike.
looks good again.
The regex looks a bit scary. ".*" matches really any sequence of characters. That's OK for a quick manual search, but should not be used in production code, because it's too easy to have it match more than expected. In this case, it would e.g. also match the following argument: ${system:NL} \"do_prompt\" ${workspace_loc} .*------------- .*---------------- A better regex would use an excluded character set containing the terminating character "}", i.e.: "\\$\\{[^}]*_prompt[^}]*\\}"
I agree, the suggested regex would be safer. But, keep in mind that the arguments are first parsed into separate arguments before the matching is done. So the case mentioned would not be an issue since we would do the matching on 3 separate cases - and none would match. ${system:NL} \"do_prompt\" ${workspace_loc}
Verified in M20090902-1400. I don't plan on further changes in 3.5.1. I will open a new bug for 3.6 to address improving the regular expression.
