Bugzilla – Full Text Bug Listing
| Summary: | Orbit bundles now contain Eclipse-SourceReferences | ||
|---|---|---|---|
| Product: | [Tools] Orbit | Reporter: | David Williams <david_williams> |
| Component: | releng | Assignee: | David Williams <david_williams> |
| Status: | RESOLVED FIXED | QA Contact: | Project Inbox <orbit.releng-inbox> |
| Severity: | normal | ||
| Priority: | P3 | CC: | aniefer, gunnar, kim.moir, mober.at+eclipse, remy.suen |
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Bug Depends on: | 331827 | ||
| Bug Blocks: | |||
|
Description
David Williams
This seems to have been introduced with bug 331827. From my little test, this didn't seem all that great. It doesn't seem like the source so imported is really valid ... the 'source-bundle' directory is pulled in as a subdirectory of the main bundle (as would be expected) but that source doesn't "compile" correctly (opening one of those .java files shows lots of errors .. invalid package name, etc.) I doubt it'd be found during debugging sessions ... but, I do not know that for sure. Adding Kim and Andrew to the CC as they may be familiar with this. David, I don't understand why the source is being imported as a subdirectory of the existing bundle. The source references allow you to import the source from the CVS etc as opposed to using the binary bundle http://relengofthenerds.blogspot.com/2010/04/wheres-source.html (In reply to comment #3) > David, I don't understand why the source is being imported as a subdirectory of > the existing bundle. The source references allow you to import the source from > the CVS etc as opposed to using the binary bundle > > http://relengofthenerds.blogspot.com/2010/04/wheres-source.html In Orbit, we have a special technique of checking in source as a subdirectory of main bundle, and then Andrew does some magic during the build to create a source bundle out of it. We do not, after all, actually build the source and couldn't treat is as a normal Eclipse project. Well, given that information perhaps Orbit is not a good use case for source references if the resulting source cannot be compiled. I think that the Eclipse-SourceReferences header is a good thing on any bundle. It really simplifies the process involved in contributing back to Eclipse. What steps are typical necessary to get the "source" of a bundle? It's a mixture of "Google/Search/Read/Browse" until potentially a page is found with some cryptic SCM info. With the Eclipse-SourceReferences it is so easy - File-Import-Plug-ins-fromRepository .. grab some coffee .. make changes .. submit patch. Although the Orbit layout is a bit different I think it's still valuable to have the information available there. It does no harm. Actually, it bakes a lifetime references to the bits from which a bundle has been built into its manifest. See bug 331827 comment 0 for why I did this - thanks Gunnar for chiming in. The main value of Eclipse source bundles is NOT providing something that is compileable. It is for providing source such that something can be debugged. By pulling in the Orbit bundle in its "original form" into a workspace, we not only support debugging the bundle but also help consumers understands where the bundle comes from such that fixes can be communicated to the right place if need be. When I committed the change, I was aware that not all bundles would pick up the change immediately unless retagged. But I thought that retagging just for the sake of the header might not be worthwile. Looks like I should have communicated the change more clearly on the Orbit dev list... I think Gunnar makes some good points ... It is just the "source" in SCM that's being provided ... not really usable source code. Guess I'd just some to think of it that way. It certainly could help some issues, but I'll point out does not help Martin's original "case b" > (b) I get the source for the binary bundle into my workspace for debugging. Or, am I missing something? Is the source in source-bundle really "attached" to the binary classes? Pretty cool, if so. As for Martin's last point: > Given that Orbit projects use multiple branches, this feature is expected to > be especially valuable since it makes it easy to get the correct branch into > the Workspace. All it gets into the workspace is a version tag. To be effective, some one still has to figure out what branch it came from ... which isn't hard ... I'm just quibbling over terminology, or else clarifying (depending on what was originally intended). So, as long as the expectation is this function simply "gets something, not necessarily compilable source, from a source code repository" then having this sounds fine to me. I was thinking the expectation was it would actually get usable source code ... but ... maybe that's just me. Assuming we leave it directive in manifest.mf, ... would we not encourage people to retag? Or just leave that up to whenever it happens? The danger is that we'd have two versions of the bundle "in the wild" with different content (but exact same version and qualifier). That's not good. The last thing someone should check out (I'd recommend those that want this function) is the effect on "pre-built" bundles ... is PDE smart enough to leave those out of this operation? Or, am I wrong about that too and still fine to modify those manifest.mf files? (I'm sure it would not be if they were signed ... but, don't think we currently have any already signed bundles). I know I seem "negative" in my remarks ... but I'm not ... I am cautious and don't word things carefully. I think this needs good review, before we put in a milestone build. I'm all for it if everyone agrees its a good thing. And, not much time left to decide. Thanks again. FYI, my concern about the prebuilt=true bundles seems unfounded. They do not have the Eclipse-SourceReference directive added to their manifest.mf file. So, not sure if that's by design, exactly, or just a coincidence of the way it all works (i.e. there would be no "manifest.mf" file on file system) but end result is good, as we shouldn't modify prebuilt bundles. FWIW, If we ever did have to exclude or manipulate things explicitly, we could probably take advantage of the "post fetch" task in customTargets.xml to manipulate the sourceReferences.properties, as Kim outlines in her blog post, http://relengofthenerds.blogspot.com/2010/04/wheres-source.html (which I found very helpful, as always :) Thanks Kim. My only lingering concern is having two versions of bundle with different content. I know we _must_ retag effected bundles (all except prebuilt) by _release_ ... only question is if we need to for the milestone?
> My only lingering concern is having two versions of bundle with different
> content. I know we _must_ retag effected bundles (all except prebuilt) by
> _release_ ... only question is if we need to for the milestone?
I've had an epiphany. If we add a p2.mirror task to our build process, using a comparator baseline (such as previous milestone) then this issue would not be an issue. We would distribute only one version of a bundle, for any given version and qualifier. As people did re-tag, the Eclipse-SourceReferences would then be in the unique, most recent bundle, and then be distributed. That way, there would not be such pressure that people would _have_ to retag, by any particular deadline.
Only problem is I'm not sure I can add this mirror step by our M6 S-build promotion, scheduled for Friday. Hence, I'd like to revert the property setting for now, and once the mirror step is added, then we could add it back.
Sound agreeable?
Especially for a milestone build, I think _having the feature in_ for getting feedback is more important than _not having it in_ for 100% consistency. Things are different in a release though. What about this (not sure if it works with the current version of PDE build): Does the PDE "Add Source References" feature have a mode in which source references are ONLY added to a bundle when that bundle has some special markup? Then any Orbit Bundle providers could opt in to the features while others could remain unchanged. I remember that when the source references support was originally implemented, I asked for such an "opt-in by bundle markup" feature. I don't know whether it was actually done. Andrew ? (In reply to comment #11) > Especially for a milestone build, I think _having the feature in_ for getting > feedback is more important than _not having it in_ for 100% consistency. > > Things are different in a release though. Feedback? What kind of feedback would you find helpful? "Leave it out"? :) Seems to me if we decide its a good idea ... then its a good idea. It is not like a feature that can be "tweaked". Its all or nothing. Also, I do think milestones should be releasable, for Orbit ... at least that's the goal. And, I do know some obsessive-compulsive adopters that check, and would ask me "hey, these jars have different contents, but same version and qualifier, what's up with that, what kind of build are you running there" (to greatly exaggerate). Plus, without the mirroring task, all 450 Orbit bundles would have to be retagged ... which seems onerous and risky. But, I am actually making good progress on getting mirroring worked in to our build, in local tests, so think I can restore property by Friday. Thanks David, you're making some very good points (as always).
The point I was coming from was that I wanted to actually test the feature on some bundles, ie perform an "Import from CVS" with PDE on some actual install. That, of course, requires the feature to be enabled in some actual builds. And so far the only build I could get hold of is my own project, using this week's Orbit I-build.... that's why I wanted to see bundles in M6 at first.
The thing that I didn't consider is that using the source reference we _know_ that what an end user gets into the workspace is _exactly_ the project as we have in our workspaces today. So there's not really a need testing the feature ... whatever works in our workspaces today will also work once the source bundle is imported. With the one caveat that PDE provides an "Import from HEAD instead" feature on import, which is misleading for Orbit... so that's a plea for bundle maintainers to keep their README.TXT updated in the HEAD stream, indicating what branches are active.
I'm also also making a case for trying to see whether we can get source attachments working for debugging inside the workspace... I haven't checked but I do think it should be possible, has anybody done it yet?
So, taking all this into account...
- the source references feature is definitely valuable and the right way to go
- I'm OK with not enabling the feature in M6 to avoid unnecessary stress
- Thanks for looking at the comparator, that's a great idea, but I'm also OK
with just keeping the change reverted in M6 to reduce stress.
I'm going to resolve this as fixed. Eclipse-SourceReferences has been re-enabled and I'm ok with it given the discussions, expectations, and improvements in our Orbit build. The build has been improved to not "re-release" something if it has the same version and qualifier as a previously "released" version. This means we won't have two bundles in the wild, but with slightly different manifests. It also means committers will have to re-tag bundles to "pick up" Eclipse-SourceReference change, if desired. For more information on p2 mirroring with comparator, see Andrew's excellent blog entry: http://aniefer.blogspot.com/2009/08/versioning-p2-slides-from-eclipsecon.html or how another project uses it: http://wiki.eclipse.org/WTP/Build/Accounting_for_history_during_builds On each Orbit download page, there will now be a "P2 Mirror Comparator Output" link with information about the mirroring process and any issues seen by the comparator. It is kind of cryptic. For example, these Eclipse-SourceReference differences show up as "The Manifest file sizes differ by 1 elements." and someone would have to download each bundle, old and new, and compare them to see exactly what was different. For example, one full message might be: !MESSAGE Difference found for canonical: osgi.bundle,org.apache.commons.pool,1.3.0.v20081016-1005 between file:/home/data/httpd/download.eclipse.org/tools/orbit/downloads/drops/S20110124210048/repository/ and file:/shared/orbit/projects/orbit-I/workdir/buildrepository/ !SUBENTRY 3 org.eclipse.equinox.p2.repository.tools 4 0 2011-03-02 15:36:21.437 !MESSAGE The Manifest file sizes differ by 1 elements. Thanks everyone. Good luck. |