Bug 324310 - Allow API baseline to be specified via a target definition
Summary: Allow API baseline to be specified via a target definition
Status: VERIFIED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: API Tools (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows XP
: P3 enhancement with 8 votes (vote)
Target Milestone: 4.5 M7   Edit
Assignee: Brian de Alwis CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on:
Blocks: 464813 466588
  Show dependency tree
 
Reported: 2010-09-02 10:18 EDT by Darin Wright CLA
Modified: 2015-09-15 02:59 EDT (History)
15 users (show)

See Also:


Attachments
Sample targets within an Eclipse project for demonstration purposes (1.46 KB, application/octet-stream)
2015-03-31 10:34 EDT, Brian de Alwis CLA
no flags Details
Snap of dialog getting hidden (164.38 KB, image/jpeg)
2015-04-22 13:46 EDT, Vikas Chandra CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Wright CLA 2010-09-02 10:18:17 EDT
I have my target platform set up as a target definition that references a p2 repository. Similarly, I have a target definition that references a p2 repository for the previous version/release of the target platform. It would be nice to be able to set up my baseline by pointing to a target definition (currently the user is limited to point to an install location only).

Better yet, it would be nice to be able to optionally associate an API baseline (target definition) with each target definition. That way, whenever the user switches targets, the API is also kept in synch.
Comment 1 Dennis Huebner CLA 2011-12-14 05:31:38 EST
(In reply to comment #0)
> I have my target platform set up as a target definition that references a p2
> repository. Similarly, I have a target definition that references a p2
> repository for the previous version/release of the target platform. It would be
> nice to be able to set up my baseline by pointing to a target definition
> (currently the user is limited to point to an install location only).
> 
> Better yet, it would be nice to be able to optionally associate an API baseline
> (target definition) with each target definition. That way, whenever the user
> switches targets, the API is also kept in synch.

Also buckminster work with target definition to specify an api-baseline. It would be nice if we could reuse one target definition file in the IDE and during the build. 
Are there any progress/thoughts on this?
Comment 2 Curtis Windatt CLA 2011-12-14 09:22:43 EST
This is not something we have resources to work on in Juno, but it is definitely something that we are interested in.  Having the API baseline be based on a target platform is reasonably straightforward, though we have to be aware of the performance impacts of a p2 based target.
Comment 3 Marc-André Laperle CLA 2013-07-18 00:13:07 EDT
This would really help new contributors. Setting up the API baseline is one of the more confusing step in getting started (at least for CDT development). If we can tell them to click on a button and it downloads both the target platform AND the API baseline using P2, it would be a big win.
Comment 4 Eclipse Genie CLA 2015-03-20 16:57:15 EDT
New Gerrit change created: https://git.eclipse.org/r/44283
Comment 5 Brian de Alwis CLA 2015-03-20 17:10:15 EDT
I've pushed up a first-cut at a solution that allows creating an API Baseline from a known target definition.  I thought of creating an ApiBaseline subclass, but the persistence piece in ApiBaselineManager makes that pretty difficult.  I instead have a specifically-formatted 'location' that uses a specific prefix to identify baselines from a target.  The location also contains the target's sequence number to detect whether the baseline may be stale in the UI.

For now I just use the normal PDE label provider to generate the labels, so they're not as pretty as the labels shown in the Target Definition prefpage.
Comment 6 Eclipse Genie CLA 2015-03-23 12:24:50 EDT
New Gerrit change created: https://git.eclipse.org/r/44390
Comment 7 Eclipse Genie CLA 2015-03-23 23:36:34 EDT
WARNING: this patchset contains 1050 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 8 Brian de Alwis CLA 2015-03-23 23:41:21 EDT
Cleaned up previous suggestion, added some tests for loading from a target platform, and create minimal pretty label provider for the target platforms.
Comment 9 Eclipse Genie CLA 2015-03-25 12:09:57 EDT
WARNING: this patchset contains 1050 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 10 Eclipse Genie CLA 2015-03-25 12:10:14 EDT
WARNING: this patchset contains 1050 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 11 Brian de Alwis CLA 2015-03-31 10:34:18 EDT
Created attachment 252041 [details]
Sample targets within an Eclipse project for demonstration purposes
Comment 12 Eclipse Genie CLA 2015-03-31 11:17:45 EDT
WARNING: this patchset contains 1048 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 13 Benjamin Cabé CLA 2015-04-17 05:41:32 EDT
I've opened CQ 9564 to get patch set #9 IP-reviewed.
Comment 14 Vikas Chandra CLA 2015-04-19 08:30:09 EDT
Hi Brian,

I think 44283 -  9 is the latest patch for this problem. I used 44390 and it appeared to be in non-working stage. Please let me know if this is correct.

Also I looked at 44283 - 9 and simple cases seemed to work OK. I will have a better look at it on Monday.

Also moving this to 4.5M7
Comment 15 Brian de Alwis CLA 2015-04-19 14:33:04 EDT
Hi Vikas.  Yes, 44283 is the right change.  44390 was a mistaken push of earlier work-in-progress and is marked as abandoned.
Comment 16 Benjamin Cabé CLA 2015-04-21 13:19:07 EDT
(In reply to Vikas Chandra from comment #14)

> Also I looked at 44283 - 9 and simple cases seemed to work OK. I will have a
> better look at it on Monday.
> 
> Also moving this to 4.5M7

Vikas, are we ready to merge this?
FWIW IP team has approved the CQ corresponding to the latest changeset.
Comment 17 Vikas Chandra CLA 2015-04-22 05:30:12 EDT
I had a look at 44283 - 9 . It looks good.

I could find out few minor issues

1) When I click Edit API base of a target , the edit dialog window goes to the background. To recreate this, target based api baseline should be selected by checkbox and also the selection of mouse should be on that target before clicking edit.

2) I cannot rename the api baseline name of a non-selected target ( non-active) without doing a Reset. I think that should be possible.

3) License year needs to be updated in all files that have been modified.

4) Edit api baseline is very slow ( not sure if we can improve performance here by caching or elsewise).

I won't consider any of them to be a NO-GO but they are more like polish issues with this enhancement.

Since approval for "44283 - 9" is already done, one  option is to merge " 44283 - 9" to masters and open a defect on these minor issues.

Dani/Brian, Please let me know your views.
Comment 18 Brian de Alwis CLA 2015-04-22 10:30:05 EDT
I'll fix up #2 and #3 now.

#1: I haven't seen this, but I can't think of how it can be related to my changes.  I'll look at it.

#4: What exactly seems slow — the 'reset'?  That would be the target resolving time?  I think everybody would love to speed that up :-)
Comment 19 Vikas Chandra CLA 2015-04-22 13:45:19 EDT
>>#4: What exactly seems slow — the 'reset'?

I just click on Edit

To recreate it

a) Have 3-4 target definitions  in workspace
b) Have couple of target based api baselines



>>#1: I haven't seen this, but I can't think of how it can be related to my changes.  I'll look at it.

To recreate it

a) Have 3-4 targets definitions in workspace
b) Have couple of target based api baseline
c) Ensure the selected target based api baseline is active too. ( see snap)
d) Click on edit
Comment 20 Vikas Chandra CLA 2015-04-22 13:46:39 EDT
Created attachment 252648 [details]
Snap of dialog getting hidden
Comment 21 Brian de Alwis CLA 2015-04-22 14:22:38 EDT
#1: Should be fixed.  Unrelated to my change, but the ApiBaselinePreferencePage was using ApiUIPlugin.getShell() rather than the preference page's shell.


#2: This is a good point.  I'm setting the baseline's location prematurely and so it is pulling out the TargetDefinition's sequence number before the TargetDefinition has been resolved.  I've moved this afterwards.

In testing this I hit a two problems.  The first is where I had two target definitions with the same name, and so the wrong target definition was subsequently picked up.  And then the "wrong one" encountered a resolving error that is logged but not communicated to the user.  I'm in the midst of fixing those.


#3: Fixed.


#4: Were these against download.eclipse.org?  I wonder if you're being bounced to a slow mirror.

Thanks for the checks!
Comment 22 Brian de Alwis CLA 2015-04-22 17:26:31 EDT
(In reply to Brian de Alwis from comment #21)
> In testing this I hit a two problems.  The first is where I had two target
> definitions with the same name, and so the wrong target definition was
> subsequently picked up.  And then the "wrong one" encountered a resolving
> error that is logged but not communicated to the user.  I'm in the midst of
> fixing those.

Both of these problems are now fixed.

I'd like to hear more about #4 as it seems snappy enough for the targets I've been loaded. At least, I'd expect it to be no slower than loading from an equivalent directory-based baseline.
Comment 23 Vikas Chandra CLA 2015-04-23 06:36:40 EDT
#1,#2 and #3 are fine. For some reason #1 happened only for target based api baselines only.

For #4, it takes 8 seconds for edit dialog to launch up and it happens for all cases - not just 1st time. So if I close the window and open again, I still get that delay.

Trial- 1
start of TargetBasedApiBaselineWizardPage::createcontrol=1429780930294
start of WorkingCopyOperation::run=                      1429780934297
end  of WorkingCopyOperation::run=                       1429780938265
end of TargetBasedApiBaselineWizardPage::createcontrol=  1429780938275

Trial - 2
start of TargetBasedApiBaselineWizardPage::createcontrol=1429780978529
start of WorkingCopyOperation::run=                      1429780981480
end  of WorkingCopyOperation::run=                       1429780985331
end of TargetBasedApiBaselineWizardPage::createcontrol=  1429780985331


All API tool test run fine. Yes, we should put more tests. 

I found another minor issue
1) create a target based baseline
2) click edit
3) you can see the target selected
4) close eclipse
5) launch again
6) click edit for target based baseline
7) selection disappears.
Comment 24 Brian de Alwis CLA 2015-04-23 10:59:44 EDT
(In reply to Vikas Chandra from comment #23)
> For #4, it takes 8 seconds for edit dialog to launch up and it happens for
> all cases - not just 1st time. So if I close the window and open again, I
> still get that delay.

8 seconds seems extraordinary.  Although I am working against an SSD, AFAICT there is no substantial computation going on in WorkingCopyOperation: it simple creates new IApiComponent instances for the bundles referenced in a baseline.

(Are your timings occurring while in the debugger?  Could you try toggling off all breakpoints — I've noticed strange slowdowns on occasion due to what seems like a corrupt or misinstalled breakpoint.  I've never been able to reproduce it though.  Deleting and re-creating the breakpoints seems the only solution.)

The WorkingCopyOperation would normally show a progress bar except that wizard pages are invisible until they've all been created (viz WizardDialog#createPageControls()).  I could change this to be done in setVisible() instead.

And I should note that WorkingCopyOperation and how its called is identical to what was used previously, so you should see the same delay if you use the directory-based installation too.


> I found another minor issue
> 1) create a target based baseline
> 2) click edit
> 3) you can see the target selected
> 4) close eclipse
> 5) launch again
> 6) click edit for target based baseline
> 7) selection disappears.

It sounds like my ITargetHandle#getMemento().hashCode() approach for disambiguating between identically-named targets is failing.  Depending on String#hashCode() as being stable between JVM launches probably isn't the best idea.  I'll rework it to insert the target handle location instead.
Comment 25 Brian de Alwis CLA 2015-04-23 11:54:08 EDT
(In reply to Vikas Chandra from comment #23)
> For #4, it takes 8 seconds for edit dialog to launch up and it happens for
> all cases - not just 1st time. So if I close the window and open again, I
> still get that delay.

I see these same delays when using the classic directory-based baselines.  I have some ideas for making these wizard pages be copy-on-write instead.  I've opened that separately as bug 465323.

(In reply to Brian de Alwis from comment #24)
> > I found another minor issue
> > 1) create a target based baseline
> > 2) click edit
> > 3) you can see the target selected
> > 4) close eclipse
> > 5) launch again
> > 6) click edit for target based baseline
> > 7) selection disappears.
> 
> It sounds like my ITargetHandle#getMemento().hashCode() approach for
> disambiguating between identically-named targets is failing.  Depending on
> String#hashCode() as being stable between JVM launches probably isn't the
> best idea.  I'll rework it to insert the target handle location instead.

I just uploaded a new patchset with three changes:

The WorkingCopyOperation specifies fork=true.  Not that this makes a difference with WizardPages.

I changed the location code to use the location from the ITargetDefinition -> ITargetHandle.  This should be stable and will also work in the face of target name changes, which is much better.

To simplify comparisons against the previous code, I renamed what was ApiBaselineWizardPage to DirectoryBasedApiBaselineWizardPage and renamed the AbstractApiBaselineWizardPage to ApiBaselineWizardPage.
Comment 26 Vikas Chandra CLA 2015-04-24 04:36:31 EDT
44283 - 12 looks fine.

Functionality works as expected. All tests runs fine. Performance issue is tracked via bug 465323 ( but not a regression since new functionality). Code also looks ok.
Comment 27 Vikas Chandra CLA 2015-04-24 04:37:03 EDT
Hi Benjamin,

Do we need a separate approval for 44283 - 12 ??


Vikas
Comment 28 Benjamin Cabé CLA 2015-04-24 04:39:11 EDT
(In reply to Vikas Chandra from comment #27)
> Hi Benjamin,
> 
> Do we need a separate approval for 44283 - 12 ??
> 
> 
> Vikas

Yes we do. I will work on that.
Comment 29 Benjamin Cabé CLA 2015-04-24 04:50:17 EDT
(In reply to Benjamin Cabé from comment #28)

> Yes we do. I will work on that.

I've opened CQ 9604.
Comment 30 Vikas Chandra CLA 2015-04-24 04:55:32 EDT
Thanks Benjamin !
Comment 32 Vikas Chandra CLA 2015-04-24 14:40:23 EDT
See previous comment for commit id.
Comment 33 Vikas Chandra CLA 2015-04-27 05:12:56 EDT
verified in 
Version: Mars (4.5)
Build id: N20150424-2000
Comment 34 Eclipse Genie CLA 2015-04-28 13:47:11 EDT
New Gerrit change created: https://git.eclipse.org/r/46705