Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 317760 - SimpleArtifactRepository behaviour change
Summary: SimpleArtifactRepository behaviour change
Status: CLOSED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: 3.6   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.6.1   Edit
Assignee: Matthew Piggott CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-23 20:38 EDT by Matthew Piggott CLA
Modified: 2010-09-16 12:07 EDT (History)
4 users (show)

See Also:
tjwatson: pmc_approved+


Attachments
Test case (5.34 KB, application/octet-stream)
2010-06-23 20:38 EDT, Matthew Piggott CLA
pascal: iplog+
Details
Patch (1.40 KB, patch)
2010-08-19 15:30 EDT, Matthew Piggott CLA
pascal: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Piggott CLA 2010-06-23 20:38:49 EDT
Created attachment 172562 [details]
Test case

Between 3.5 and 3.6 releases there is a behaviour change in retrieving packed artifacts in repositories with publishPackFilesAsSiblings=true, but artifact.uuid set on certain artifacts.

In Eclipse 3.5 we would use the UUID and retrieve the artifact from the blobstore however in 3.6 we attempt to retrieve the artifact from plugins.

I've attached a test along with a repository which should demonstrate the problem, minor changes are required between the two version of Eclipse.
Comment 1 Igor Fedorenko CLA 2010-07-02 09:17:25 EDT
This problem was originally reported as https://issues.sonatype.org/browse/TYCHO-464
Comment 2 Pascal Rapicault CLA 2010-08-18 19:51:17 EDT
Matt, any patch in hand?
Comment 3 Matthew Piggott CLA 2010-08-19 15:30:49 EDT
Created attachment 177040 [details]
Patch

In the patch I've reversed the check for an artifact UUID ahead of the flatButPackedEnabled check.
Comment 4 Pascal Rapicault CLA 2010-08-21 19:10:19 EDT
The change of behaviour observed here comes from the fix for bug #306056. The fix that Matt suggests ends up putting the code back to the same state than what we shipped in eclipse 3.5. This code would be consistent with the createLocation() method. 

Unfortunately that breaks the test written for bug #306056 (ArtifactMirrorApplicationTest#testArtifactMirrorUUID_bug306056), but in retrospect I'm questioning the validity of the fix for 306056. Indeed there was two parts quoting: 

"Two things:
1) the destination repo should probably clear the UUID if it isn't using the
blobstore
2) when getting the artifact, we should check publishPackFilesAsSiblings before
checking the UUID"

The first part of the fix is definitely correct, however the second part cater for the case we have had bad repositories resulting from bug fixed by the first part of the fix. 
I'm not sure that we should have done this.

What do you guys think?
Comment 5 Andrew Niefer CLA 2010-08-24 15:06:38 EDT
(In reply to comment #4)

> 2) when getting the artifact, we should check publishPackFilesAsSiblings before
> checking the UUID"
> 
> The first part of the fix is definitely correct, however the second part cater
> for the case we have had bad repositories resulting from bug fixed by the first
> part of the fix. 
> I'm not sure that we should have done this.
> 
> What do you guys think?

It doesn't really matter to me if this part is reverted.
In this case, we really have a repository that is not internally consistent, if using the UUID gets better results from the user's end, then that is probably the way to go.
Comment 6 Thomas Watson CLA 2010-09-01 11:17:08 EDT
3.6.1 is pretty much done at this point.  Can we defer this bug?
Comment 7 Thomas Watson CLA 2010-09-01 11:45:01 EDT
Discussed with Pascal.  We need to get this in for the RC3 rebuild.  Pascal, please post a note to the releng list about this fix we are including in the respin.
Comment 8 Pascal Rapicault CLA 2010-09-01 12:39:46 EDT
I've released Matt fix and removed the test that this change was breaking. Fixed in HEAD and the branch.
Comment 9 DJ Houghton CLA 2010-09-16 12:07:54 EDT
There was a problem with tagging HEAD so this fix won't appear in integration builds until the first build after 3.7 M2.