Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 317796 - Build before launch should be cleverer about which configuration is built
Summary: Build before launch should be cleverer about which configuration is built
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug (show other bugs)
Version: 7.0   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 8.0   Edit
Assignee: James Blackburn CLA
QA Contact: Ken Ryall CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-24 05:53 EDT by James Blackburn CLA
Modified: 2012-05-22 20:19 EDT (History)
3 users (show)

See Also:
john.cortell: review+
jamesblackburn+eclipse: review? (ken.ryall)


Attachments
patch 1 (14.03 KB, patch)
2010-08-06 10:48 EDT, Alex Collins CLA
no flags Details | Diff
patch 2 (16.38 KB, patch)
2010-08-11 07:07 EDT, Alex Collins CLA
jamesblackburn+eclipse: iplog+
Details | Diff
patch 3 (19.62 KB, patch)
2010-08-23 07:10 EDT, James Blackburn CLA
no flags Details | Diff
patch 4 (19.98 KB, patch)
2010-08-23 07:58 EDT, James Blackburn CLA
cdtdoug: iplog-
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Blackburn CLA 2010-06-24 05:53:42 EDT
The Main tab has a drop down to select the build configuration to build before launch.  It's populated with:
  - Use Active
  - Config 1
  - Config 2
  - etc...

For a new launch config, it defaults to choosing the currently Active configuration (which may be different to the configuration which produced the binary).

With an integrated build system we should be able to do better: it should be possible to have an "Automatic" entry which builds the configuration which generates the build artifact that will be launched.  
When there's more than one configuration "Use Active" can produce misleading results confusing users.
Comment 1 Alex Collins CLA 2010-08-06 10:48:27 EDT
Created attachment 176043 [details]
patch 1

The attached patch adds a checkbox to the launch configuration dialog, enabling or disabling automatic choosing of the configuration to be built, based on the path to the program.

When the checkbox is enabled the build config drop down is greyed out, and the configuration that is automatically chosen is selected. This shows the user which configuration chosen before running the launch.

The choice of build config is updated whenever the launch is run, to respect any intervening changes to the build configurations.
Comment 2 James Blackburn CLA 2010-08-10 12:04:21 EDT
A couple issues:
  - java.lang.NullPointerException
at org.eclipse.cdt.launch.LaunchUtils.getBuildConfigByProgramPath(LaunchUtils.java:178)
at org.eclipse.cdt.launch.ui.CAbstractMainTab.updateBuildConfigCombo(CAbstractMainTab.java:287)
at org.eclipse.cdt.launch.ui.CAbstractMainTab.updateBuildOptionFromConfig(CAbstractMainTab.java:417)
at org.eclipse.cdt.dsf.gdb.internal.ui.launching.CMainTab.initializeFrom(CMainTab.java:276)
   when opening the dialog (ACPathEntry#getLocation() returns null if the directory doesn't exist).
 - private CAbstractMainTac#fBuildConfigAuto perhaps make this protected to be consistent with the other GUI members.
 - 'Auto' not checked by default
 - Checkbox label "Choose automatically using program name" perhaps better that the label contains the text field's name: "Select configuration using 'C/C++ Application'"?
 - LaunchUtils#getBuildConfigByProgramPath. Is findFilesByName the right thing to do for relative paths? Aren't relative paths always relative to the project? Probably something like IProject.getFile(path).getLocation()
    + '// Matched more than one, so use the active configuration' this is unlikely to happen, perhaps log this?
 - What happens when a non-CDT project is used?
Comment 3 Alex Collins CLA 2010-08-11 07:07:44 EDT
Created attachment 176324 [details]
patch 2

Thanks for the feedback James; the attached patch resolves these issues.
Comment 4 James Blackburn CLA 2010-08-23 07:10:04 EDT
Created attachment 177203 [details]
patch 3

A few tweaks to Alex's patch:
  - LaunchUtils#getBuildConfigByProgramPath: should use core model for project description, not property manager (which can only be safely used from UI thread).
  - AbstractCLaunchDelegate2#buildForLaunch: should also check the ATTR_PROJECT_BUILD_CONFIG_AUTO flag and select the appropriate build configuration
  - dsf.gdb.ui CMainTab: set auto config as default for new configurations here too

It's slightly odd that LaunchUtils#getBuildConfigByProgramPath converts resources to location for comparison rather than sticking to IResources, but this looks consistent with the CMainTab. Given the launches only work on local resources, I guess this is ok for now.
Comment 5 James Blackburn CLA 2010-08-23 07:11:48 EDT
John / Ken I think you were involved in the build before launch functionality. If you get a chance, review would be appreciated.
Comment 6 James Blackburn CLA 2010-08-23 07:58:54 EDT
Created attachment 177205 [details]
patch 4

Don't change the underlying launch configuration (i.e. don't apply the auto attribute) if the user hasn't explicitly clicked the checkbox. This is to prevent unnecessary deltas of launch configs held in a repository.
Comment 7 James Blackburn CLA 2010-08-26 11:36:34 EDT
Thanks for the patch Alex. Committed to HEAD (8.0).
Comment 8 CDT Genie CLA 2010-08-26 12:23:02 EDT
*** cdt cvs genie on behalf of jblackburn ***
Bug 317796 -  Build before launch should be cleverer about which configuration is built.
  Add option to LaunchConfig Main Tab to automatically choose a build configuration appropriate to the binary being debugged.

[*] CMainTab.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/launching/CMainTab.java?root=Tools_Project&r1=1.12&r2=1.13

[*] ICDTLaunchConfigurationConstants.java 1.34 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-debug/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/ICDTLaunchConfigurationConstants.java?root=Tools_Project&r1=1.33&r2=1.34

[*] LaunchMessages.properties 1.36 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-launch/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/internal/ui/LaunchMessages.properties?root=Tools_Project&r1=1.35&r2=1.36

[*] CMainTab.java 1.87 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-launch/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/ui/CMainTab.java?root=Tools_Project&r1=1.86&r2=1.87
[*] CAbstractMainTab.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-launch/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/ui/CAbstractMainTab.java?root=Tools_Project&r1=1.2&r2=1.3

[*] LaunchUtils.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-launch/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/LaunchUtils.java?root=Tools_Project&r1=1.9&r2=1.10
[*] AbstractCLaunchDelegate2.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-launch/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/AbstractCLaunchDelegate2.java?root=Tools_Project&r1=1.9&r2=1.10
[*] AbstractCLaunchDelegate.java 1.73 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-launch/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/AbstractCLaunchDelegate.java?root=Tools_Project&r1=1.72&r2=1.73

[*] MANIFEST.MF 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt-launch/org.eclipse.cdt.launch/META-INF/MANIFEST.MF?root=Tools_Project&r1=1.17&r2=1.18
Comment 9 John Cortell CLA 2010-09-27 14:37:18 EDT
Nice patch. Looks good to me.