Bug 325489

Comment 1 John Arthorne 2010-09-30 09:17:51 EDT

Have you considered mapping project configurations or build variants to separate builders on the project?  This seems closer to your desired behaviour because:

 - Each individual builder on a project receives its own unique delta
 - Each builder has a Map of arguments that could allow specifying what configuration it corresponds to
 - Each builder has its own unique set of project references, as specified by the return value of the IncrementalProjectBuilder#build method.

I like the build context idea though, for passing in additional information about what the build manager is up to (project build or workspace build, what has been built already, etc).

Comment 2 James Blackburn 2010-09-30 09:49:03 EDT

Yes I did consider this initially, and it nearly works. 

There are a few problems:
  - Builders can't easily work together: Without a common API multiple builders, from different integrators, can't work in concert to produce build output for a given variant.
  - No API for building a project and its references.  What you've suggested pushes the complexity for managing the build to every caller of the core.resources build API.  
      + Build before launch.  
      + BuildAction UI needs to be specialised (once for each integration...) 
        when the wrong action gets invoked the user ends up with inconsistent build output.
  - No necessarily a bijection between the project variant graph and project reference graph -- so build order is context dependent.
  - What does IProject.build do?
  - What does IWorkspace.build build?

What we've proposed isn't far from what you've suggested.  We've just tried to formalise it with API to make it much easier to leverage the platform functionality.  If there's some namespace for variants, and references can be defined between them, we can:
  - add methods to build a project and its reference graph
  - make it easy to write variant aware builders

It's not just a case of getting a good delta (which is important) but also a case of controlling the build process, and making it easier for build API users to build what they want.

Comment 3 John Arthorne 2010-09-30 10:35:24 EDT

Ok, it sounds interesting. I'll wait to see the code. My only suggestion at this point is:

 - Within the implementation, try to leverage the fact that each builder object tracks its own delta today. The delta tracking code (and associated serialization across sessions) is quite complex so wouldn't want to spread deltas around to some new place.
 - The term 'project variant' to me doesn't capture the goal here. Something like 'build variant', 'build configuration', etc, would seem more fitting.
 - Tracking a delta over the long term can get quite expensive, as the delta will continue growing. If these configurations are things that are only used occasionally you might still need to eventually discard the delta. For example if a given configuration isn't used for a long time, discard it and require a full build.

Comment 4 Doug Schaefer 2010-09-30 12:23:15 EDT

(In reply to comment #3)
>  - The term 'project variant' to me doesn't capture the goal here. Something
> like 'build variant', 'build configuration', etc, would seem more fitting.

+1, at the end of the day, we are talking about mapping CDT build configurations to these. I think 'build configuration' is a good term to use and I think it makes sense outside the CDT context as well.

Comment 5 James Blackburn 2010-10-05 10:17:06 EDT

Created attachment 180243 [details]
Tests patch 1

Tests for the new API.
All existing core.resources tests pass without change.

Comment 6 James Blackburn 2010-10-05 10:33:09 EDT

Created attachment 180247 [details]
core.resources patch 1

Initial build configurations API proposal.

Where possible the new API should complement the existing API. So we've worked hard to:
  - Not change .project if new API isn't used
  - Have WS .metadata that is both forwards and backwards compatible with existing eclipse 3.x
  - Not change any existing tests -- we've only added to the testsuite. 

Any and all feedback appreciated.

Notes:
  - All projects have one or more configurations.  Each project has a default configuration at the beginning of time. It's impossible to remove all the configurations from a project.
  - All projects have a currently 'active' build configuration. This is the configuration which is built when you call IProject.build.  If unspecified, this defaults to the first configuration in the project.
  - References between build configuration can be defined in terms of absolute build configuration ids, or a build configuration may reference the active configuration of another.
  - Project level references have been implemented in terms of build configuration references to active configuration in other project.  If there are no configuration level references, then the .project should be unchanged.
  - Where builders support configurations, additional delta trees need to be serialized in the workspace metadata.  The existing format is somewhat extensible, so I believe we've extended it in a way that's both forward and backwards compatible with existing eclipse 3.x.
  - Where builders don't support configurations, they receive one delta as before which is the delta since they were last built.
  - Current active configuration is stored as a workspace persistent property. This was done to prevent delta'ing the .project whenever tooling changes the active configuration.  (If no active configuration specified, the first is chosen).
  - References between build configurations: may want this to be API on IBuildConfiguration rather than project description

Comment 7 James Blackburn 2010-10-05 11:59:35 EDT

John/Doug, I believe the patch addresses your comments.

FYI I've put both patches onto github if you're interested in pulling them:
http://github.com/jamesblackburn/eclipse-core-resources
http://github.com/jamesblackburn/eclipse-core-tests-resources

McQ suggested that the work may get better exposure in e4. Would it be appropriate to commit these patches into there?

Comment 8 John Arthorne 2010-10-08 11:12:00 EDT

(In reply to comment #7)
> McQ suggested that the work may get better exposure in e4. Would it be
> appropriate to commit these patches into there?

If the feature was pervasive like the virtual folder stuff last year, that would make sense. My inclination here is that the change, although significant, is fairly isolated and we should be able to work it directly in 3.7 without too much destabilization.

Comment 9 John Arthorne 2010-10-08 11:42:14 EDT

Why is the patch obsolete - is there a new one coming?

Comment 10 Szymon Brandys 2010-10-08 11:52:18 EDT

We should have it released early in M4. By that time let's have couple iterations to make sure that it is ready. As John suggested let's work against 3.7 and don't move the effort to e4.

Comment 11 James Blackburn 2010-10-08 12:01:07 EDT

(In reply to comment #9)
> Why is the patch obsolete - is there a new one coming?

Yes, at some point over the next couple days.  

I'm just fixing some implementation performance nits, so I would definitely appreciate any comments you have as the API: naming, visibility, location, etc.  As well as any implementation problems you spot.

Currently fixed a couple issues in the BuildContext (which is over-engineered in patch1) as well as lots of unnecessary cloning of the configurations array.  I'll likely push the BuildContext stuff into a separate bug as it's orthogonal functionality.

We've hooked up CDT and it's building a pretty complex workspace with 76 inter-dependent projects, 220 build configurations and 1315 references between them!

If you have any comments on patch1, do shout!

Comment 12 James Blackburn 2010-10-13 11:44:33 EDT

Created attachment 180782 [details]
Tests patch 2

 - Remove tests for the build context which will be in a different bug.

Comment 13 James Blackburn 2010-10-13 11:58:51 EDT

Created attachment 180783 [details]
core.resources patch 2

Patch 2
  - Minimize unecessary delta in existing classes
  - ProjectDescription check Map<String, IBuildConfigReference[]> equals correctly
  - BuildConfiguration equals / hashcode needn't include IProject
  - Remove some unnecessary build configuration cloning.
  - Remove BuildContext

Comment 14 John Arthorne 2010-11-02 17:29:46 EDT

Here are some initial comments in random order of importance:

The patch doesn't compile in Java 1.4 or Java 5. It uses Java 6 methods, mainly Arrays.copyOf, which is only a minor convenience on top of System.arraycopy. I'm open for discussion on moving to Java 5 though - personally I think we should just switch to Java 5 immediately for core.resources.

There is a compile error in core.tests with the patch applied. This is a new test introduced since this patch was attached.

BuildConfiguration hash code doesn't consider project name, but equals method does. This isn't an error but it should be consistent.

Doe we need getDanglingBuildConfigReferences? I don't know of anyone who has ever used IWorkspace#getDanglingReferences. I don't find a single reference to it in bugzilla. I suggest not introducing getDanglingBuildConfigReferences unless we have a demonstrated need for it (although the effort for consistency with existing API is appreciated). I think we imagined a need for this in Eclipse 1.0 but it never materialized.

Do we need both dynamic and static build configuration references in ProjectDescription? Our past experience has been that static references often don't cut it, as this information is typically derived and generation from some other source (classpath, bundle manifest, etc). The static references are generally brittle and create portability problems. I'm just wondering if you see a need for both concepts, or if you are just following the pattern of traditional project references by introducing both static and dynamic build config references?

I'm wondering if the IBuildConfigReference type is needed at all. It seems awkward to use it:

IBuildConfigReference ref = p1.newReference();
ref.setConfigurationId(configId);
IProjectDescription desc = p2.getProjectDescription();
desc.setReferencedProjectConfigs(configId, new IBuildConfigReference[] {ref});

It seems simpler to just have IProjectDescription methods like:

void setReferencedProjects(String configId, IProject[] refs)
IProject[] getReferencedProjects(String configId)

I.e., the fact that IBuildConfigReference contains the configId is redundant, since you need to know the config id in order to ask for the references. Note we may still need a BuildConfigurationReference class internally to keep track of the association.

That's all I have time for today... more tomorrow hopefully.

Comment 15 James Blackburn 2010-11-03 08:16:56 EDT

Created attachment 182284 [details]
code + tests patch 3

> BuildConfiguration hash code doesn't consider project name, but equals method
> does. This isn't an error but it should be consistent.

This is because BuildConfiguration#project isn't final, and this is something that has been troubling me...  When BuildConfigs are created on the IProjectDesc the IProject isn't known. As I want to be able to compare newly created build configurations with the live build configurations in the Project, I made #hashCode only use the Id, and equals only compare the IProject if both configurations have an associated IProject.  
I guess this should be fixed (see below...).

> I'm wondering if the IBuildConfigReference type is needed at all. It seems
> awkward to use it:  ...
> I.e., the fact that IBuildConfigReference contains the configId is redundant,
> since you need to know the config id in order to ask for the references. Note

The reference is between BuildConfigurations and not at the project level. So in fact there are two configuration IDs in play.

Maybe we could change the API by adding:

  void setReferencedConfigurations(IBuildConfiguration[] configurationIds)
  IBuildConfiguration[] getReferencedConfigurations()

to IBuildConfiguration?  
The main problem here is that it's not currently possible to create a BuildConfiguration with a project which doesn't exist (whereas creating a BuildConfigReference is on IProject...)

This can be fixed together with the BuildConfiguration#equals/hashCode by moving #newBuildConfiguraiton form IProjectDesc to IProject... 

> Do we need both dynamic and static build configuration references in
> ProjectDescription? 

Good point. We need to store the configuration references somewhere. Currently they live in a CDT metadata file and are reconciled into the project description.  I thought we might move this from .cproject -> .project, so I was using static references.  However I can't think of a reason not to use the dynamic references, and continue to use .cproject as storage. Looks like the the patch, as it stands, doesn't correctly persist / restore the dynamic configuration references -- will fix this.

Along a similar vain, I'm not too attached to:
IWorkspace#computeProjectBuildConfigOrder
as API. 
From my POV the important thing to have is IWorkspace#build(... IBuildConfiguration[] ...) and clients should use that rather than re-implementing the build logic themselves.

Fixed the other things you mentioned.

Comment 16 James Blackburn 2010-11-03 08:35:09 EDT

Created attachment 182288 [details]
cdt-config-reconcile example

Example of how CDT configures platform build configurations and references.

Comment 17 John Arthorne 2010-11-03 14:36:26 EDT

(In reply to comment #15)
> The reference is between BuildConfigurations and not at the project level. So
> in fact there are two configuration IDs in play.
> 
> Maybe we could change the API by adding:
> 
>   void setReferencedConfigurations(IBuildConfiguration[] configurationIds)
>   IBuildConfiguration[] getReferencedConfigurations()
> 
> to IBuildConfiguration?  

Yes I completely missed that there were two build configuration ids at play there. Here's another idea: make IBuildConfiguration an immutable object containing configId,project,name. Discard IBuildConfigReference and let IBuildConfiguration take its place (they are essentially the same object already). That way the API works like this:

IBuildConfiguration config1 = p1.getBuildConfiguration(configId1);
IProjectDescription desc = p2.getProjectDescription();
desc.setReferencedProjectConfigs(configId, new IBuildConfiguration [] {config1});

I.e., a configuration object could be used to represent a reference as well.

One thing I missed last night is IProject#setActiveBuildConfiguration. It's unusual for a setter to be on IProject, and there are some implications here... Since it doesn't appear in the project description, that information can't be included in deltas. I'm not sure how valuable this is, but it means people can't be notified when the active configuration changes. Also I would suspect such an operation should cause an autobuild to be kicked off, since it affects the build order. It would likely be better to record the active configuration in the project description itself, so that a client can add a configuration and make it active in a single resource change operation.

Comment 18 John Arthorne 2010-11-03 14:38:41 EDT

I also forgot to add that overall this patch looks great. It's clearly well thought out from the API through to the tests and persistence, even providing bi-directional workspace compatibility. I think we are really close and we'll be able to get this in place relatively early in M4. Szymon is also planning to take a close look at it soon.

Comment 19 Szymon Brandys 2010-11-04 08:56:12 EDT

(In reply to comment #18)
> Szymon is also planning to take a close look at it soon.
Right, doing it today.

Comment 20 Szymon Brandys 2010-11-08 05:30:16 EST

(In reply to comment #19)
Could we use IProjectDescription#setBuildConfigurations(String[]) instead of IProjectDescription#setBuildConfigurations(IBuildConfiguration[]) to set configurations?

Another approach would be to have IProjectDescription#newBuildConfiguration(String id, String name)

Either way, we could get rid of setters from IBuildConfiguration.

Moreover I'm not sure if a human readable name in IBuildConfiguration is the right way to go. I guess that the name should be localized and rather handled by the UI part of Build Configurations. I recall that resource filters work this way.

If use only IBuildConfiguration without IBuildConfigurationReference, we could get rid of IProject#newReference.

>> The main problem here is that it's not currently possible to create a
>> BuildConfiguration with a project which doesn't exist (whereas creating a
>> BuildConfigReference is on IProject...)

Maybe we could use IWorkspace#createBuildConfiguration(String projectName, String configurationId, <String configurationName>) instead?

Comment 21 Szymon Brandys 2010-11-08 06:40:02 EST

(In reply to comment #14)
> The patch doesn't compile in Java 1.4 or Java 5. It uses Java 6 methods, mainly
> Arrays.copyOf, which is only a minor convenience on top of System.arraycopy.
> I'm open for discussion on moving to Java 5 though - personally I think we
> should just switch to Java 5 immediately for core.resources.

There is a bug for moving to 1.5, see Bug 311012.

Comment 22 James Blackburn 2010-11-08 06:57:08 EST

Created attachment 182600 [details]
code + tests patch 4

This is the current implementation. All tests pass.

Still to do: Investigate merging IBuildConfig + IBuildConfigRef

Any further critique on API and implementation details appreciated.

Changes:

 * IBuildConfiguration: make IProject final + test for project move
 * ProjectDescription: Persist dynamic config references  in local meta area + test
   o active configuration should be part of project description dynamic state + test
 * IWorkspace: Remove API for build config order
 * Workspace: cache the build order
   o When build order in use, projects not specified built in reference order, not name order...
 * Don’t build if project doesn’t contain a matching config for IProject#build
 * JavaDoc
 * Removed IncProjBuilder#hasBeenBuilt(IBuildConfiguration) -- this will be discoverable using IBuildContext.
 * Remove static config references. 
 * Cache dynamic config references separately to dynamic project references. Configuration level references now defined to take precedence over project level references when computing the build order.  So precedence order is:
  - Dynamic build configuration references
  - Static project level references
  - Dynamic project level references

Comment 23 James Blackburn 2010-11-08 09:47:13 EST

Created attachment 182618 [details]
code + tests patch 5

I've unified the IBuildConfigReference into IBuildConfiguration.

API tooling shows the following as additional API:

IBuildConfiguration

IncrementalProjectBuilder.getBuildConfiguration()
IncrementalProjectBuilder.rememberLastBuiltState() 

IProject.build(IBuildConfiguration, int, IProgressMonitor) IProject.getActiveBuildConfiguration() 
IProject.getBuildConfiguration(String)
IProject.getBuildConfigurations()
IProject.getReferencedBuildConfigurations(IBuildConfiguration) has been added 
IProject.getReferencingBuildConfigurations(IBuildConfiguration)
IProject.hasBuildConfiguration(IBuildConfiguration)

IProjectDescription.setActiveBuildConfiguration(String) IProjectDescription.setBuildConfigurations(IBuildConfiguration[]) 
IProjectDescription.setDynamicConfigReferences(String, IBuildConfiguration[]) 
IProjectDescription.getDynamicConfigReferences(String) 

IResourceStatus.BUILD_CONFIGURATION_NOT_FOUND 

IWorkspace.build(IBuildConfiguration[], int, boolean, IProgressMonitor) 
IWorkspace.newBuildConfiguration(String, String, String)

Comment 24 James Blackburn 2010-11-08 13:48:09 EST

Created attachment 182646 [details]
code + tests patch 6

Last patch for today - minor textual difference over last patch ...

Fix + test for two issues when integrating with the CDT builder:
    * #setBuildConfigurations caused dynamic config references to be lost + test
    * IWorkspace#build(IBuildConfiguration, true...) throws CoreException when referenced project / configuration isn’t accessible + test

Now builds my large CDT workspace once more.

What looks weird though is that:
public IBuildConfiguration[] getReferencedBuildConfigurations(IBuildConfiguration config) throws CoreException;
includes all references: dynamic-configuration, dynamic-project, static project. This looks too course as we previously used project level references as the union for all configurations, and so now need (I guess) to remove static references when fixing up the build configuration order.  
I wonder whether it would be better for getAllBuildConfigReferences to deal with just dynamic references.

Comment 25 James Blackburn 2010-11-10 08:43:28 EST

Created attachment 182810 [details]
patch 7 + tests

Patch 7 which contains API cleanup discussed with Szymon.

 * getConfigurationId() -> getId()
 * Remove ‘Dynamic’ from API get/setBuildConifgReferences
 * Remove IProject#getReferencingBuildConfigurations()
 * Persist BuildConfigurations in the LocalMetaArea not IProject
 * Tidy: JavaDoc ; method order ; internal methods

There are no longer any changes to .project - all the build configuration state is stored in the workspace metadata.
All tests pass.

Comment 26 Szymon Brandys 2010-11-10 09:57:20 EST

I think that API is fine. There are still questions like:
- where #getBuildConfiguration methods should live, IProject or IProjectDescription
- should we have IProject in IBuildConfigration. If not, what is the best way to run IWorkspace#build with configurations.

Anyway, I don't think that this questions prevent us from releasing it to HEAD. I plan to spend some time on Friday to look closer at the implementation. So far it looks fine.

Comment 27 James Blackburn 2010-11-10 11:05:14 EST

We discussed at some length whether we could get away without having an additional IBuildConfiguration type.  The main reason is that to model the tuple (IProject, ConfigurationId) without this is very messy.

(In reply to comment #26)
> I think that API is fine. There are still questions like:
> - where #getBuildConfiguration methods should live, IProject or
> IProjectDescription

The #get methods live on IProject mainly because IBuildConfiguration contains a reference to IProject. It seems reasonable that #get should go through an IProject handle.

> - should we have IProject in IBuildConfigration. If not, what is the best way
> to run IWorkspace#build with configurations.

Not sure there's a better answer for this...

Thanks Szymon for taking the time!

Comment 28 James Blackburn 2010-11-12 09:40:03 EST

Created attachment 182998 [details]
patch 8 + tests

Further tweaks for Szymon.

- For the moment remove human readable 'name' from BuildConfiguration
- Merge WorkspaceTreeReader_3 with _2 as the change isn't breaking
- Remove redundant .isEmpty() check from ProjectDesc.setBuildConfigs()

Comment 29 James Blackburn 2010-11-12 10:21:07 EST

Created attachment 183001 [details]
patch 8 + tests

Last patch was missing WorkspaceTreeReader_3 removal change ...

Comment 30 James Blackburn 2010-11-12 10:42:42 EST

Created attachment 183004 [details]
patch 8++

JavaDoc: remove mention of Workspace version 3 metadata.

Comment 31 Szymon Brandys 2010-11-12 13:06:28 EST

'patch 8++' is in HEAD. I also added two tags 'pre_Bug325489' and 'post_Bug325489' on core.resources and core.tests.resources. The first tag for sources before the patch was released, the second right after releasing it. We plan to do some further work in API and implementation, but separate bugs will be raised for that.

Comment 32 Szymon Brandys 2010-11-12 13:10:21 EST

I forgot... Thanks James! :)

Comment 33 Doug Schaefer 2010-11-12 15:06:45 EST

Very cool! Thanks Szymon, John, and of course James!