Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 332655

Summary: Setting IU properties for missing IUs in a profile dirties the profile but does not persist the property
Product: [Eclipse Project] Equinox Reporter: Dean Roberts <dean.t.roberts>
Component: p2Assignee: Pascal Rapicault <pascal>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: dean.t.roberts, dj.houghton, john.arthorne, pascal
Version: 3.7   
Target Milestone: 3.7 M5   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 323888    
Attachments:
Description Flags
patch none

Description Dean Roberts CLA 2010-12-15 11:07:55 EST 
Build Identifier: 

While investigating Bug 323888 I came across a use case that causes the planner to attempt to set IU properties for an IU that is not currently part of or being added to the profile.

When this happens, the property is set on the profile and the profile is marked as dirty in Profile#setInstallableUnitProperty.  However, ProfileWriter only persists IU properties for IUs that exist in the profile in the method ProfileWriter#writeInstallableUnitsProperties

The result is that under certain circumstances we can (and do) end up writing an installation history profile that has no changes except for the timestamp, since the properties that dirtied the profile are not persisted.

There is certainly a defect, but the nature of the defect dependant on expected behaviour.

If we should be able to set properties on IUs that are not in the profile, then the defect is in ProfileWriter.  If it is invalid to set properties on IUs that are not in the profile, then the defect is in or around Profile#setInstallableUnitProperty

Could someone please comment on whether or not we should be allowed to set properties on IUs not in the profile.

Reproducible: Always
Comment 1 Dean Roberts CLA 2010-12-15 11:20:09 EST 
There may be many paths that would cause the planner to attempt to set properties on an IU that is not part of the profile.  But here is one way to do it.

1) Have multiple versions of a singleton bundle in the drop-ins directory.
2) Restart so that the most relevant version of the bundle is installed.

From this point on every time the UI is used to install a new bundle, the
install will create two profiles.  These two new profiles will differ only in
timestamp.

This happens since the ProfileSynchronizer will treat the non-installed versions of singletone bundle as additions.  Specifically it will be trying to set properties on the non-installed IUs.
Comment 2 DJ Houghton CLA 2010-12-15 17:36:08 EST 
I am leaning towards not allowing properties to be set on IUs which aren't part of the profile but we have to be careful how we enforce this. The operands might be executed in a different order (setProperty before installIU versus vice versa) so I don't know if we can just add a check in the #setProperty to first check to see if the IU is there.

Currently we execute the operands in a specific order, but I don't believe that is spec'd.
Comment 3 Pascal Rapicault CLA 2010-12-16 20:36:42 EST 
The planner code was incorrect as it was systematically creating an operand for the properties that were not part of the solution. I have released a fix and a test.
Comment 4 DJ Houghton CLA 2010-12-17 08:16:06 EST 
Dean, the code didn't make it into the nightly build last night but you can still re-run your scenarios and see what profiles are created by grabbing last night's build and replacing the org.eclipse.equinox.p2.director JAR with one you export with code from HEAD. (changes are isolated to just this bundle)
Comment 5 Pascal Rapicault CLA 2010-12-17 09:07:46 EST 
Btw, I'm just thinking that I have not checked what happens in the engine if there is no operands (array of length 0). We may need to add a check in Engine.perform().
Comment 6 DJ Houghton CLA 2010-12-17 13:48:26 EST 
We currently check to make sure the operands isn't null. Do you mean just adding a:
  if (operands.length == 0) return;
to the beginning of the method?
Comment 7 Pascal Rapicault CLA 2010-12-17 15:16:06 EST 
yep
Comment 8 DJ Houghton CLA 2010-12-20 10:31:28 EST 
Created attachment 185553 [details]
patch

Patch to check and see if the operands are 0 length.
Comment 9 DJ Houghton CLA 2010-12-20 10:31:56 EST 
Patch released to HEAD.