Bugzilla – Full Text Bug Listing
| Summary: | Freeze report should mention compatible changes | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] PDE | Reporter: | Markus Keller <markus.kell.r> | ||||||||||||
| Component: | API Tools | Assignee: | Vikas Chandra <Vikas.Chandra> | ||||||||||||
| Status: | RESOLVED WONTFIX | QA Contact: | |||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | daniel_megert, Lars.Vogel, Olivier_Thomann, Vikas.Chandra | ||||||||||||
| Version: | 4.6 | ||||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | stalebug | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 492238, 492337 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Markus Keller
The freeze report also missed a change in a static final field declaration (from non-constant to compile-time constant). The change was reverted in master with https://git.eclipse.org/r/#/c/70032/ , but it was present in N20160406-2000 although the report missed it: http://download.eclipse.org/eclipse/downloads/drops4/N20160406-2000/apitools/freeze_report.html => known undetected API changes: - method signature change from array to varargs - static final field change from non-constant to compile-time constant I can take a quick look at this if you want, Markus. (In reply to Olivier Thomann from comment #2) > I can take a quick look at this if you want, Markus. That would be great! With my current set of changes, new entries are needed in the freeze exclude list. org.eclipse.compare.core:org.eclipse.compare.core:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.compare.win32:org.eclipse.compare.win32:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.compare:org.eclipse.compare:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.core.net.win32.x86_64:org.eclipse.core.net.win32.x86_64:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.core.net:org.eclipse.core.net:EXECUTION_ENVIRONMENT=CDC-1.1/Foundation-1.1 org.eclipse.core.net:org.eclipse.core.net:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.jsch.core:org.eclipse.jsch.core:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.jsch.ui:org.eclipse.jsch.ui:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.team.core:org.eclipse.team.core:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.team.cvs.core:org.eclipse.team.cvs.core:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.team.cvs.ssh2:org.eclipse.team.cvs.ssh2:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.team.cvs.ui:org.eclipse.team.cvs.ui:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.team.ui:org.eclipse.team.ui:EXECUTION_ENVIRONMENT=J2SE-1.4 org.eclipse.ui.net:org.eclipse.ui.net:EXECUTION_ENVIRONMENT=J2SE-1.4 These are the two entries related to compile time constants that are not required to be added since this is fixed: org.eclipse.jdt.core:org.eclipse.jdt.core.ToolFactory#M_FORMAT_EXISTING org.eclipse.jdt.core:org.eclipse.jdt.core.ToolFactory#M_FORMAT_NEW This is the entry that would have been displayed for the array to varargs change: org.eclipse.jface:org.eclipse.jface.viewers.StructuredSelection#StructuredSelection([Ljava/lang/Object;)V So both issues would now be returned. The execution environment entries above are required in the exclude list. Patch to follow. Created attachment 260814 [details]
Proposed patch
I delivered the fix. Thanks Olivier. For reference I think this is the relevant commit: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=23572f6241d42dbeddb6371c0b3a822c5d6bd658 We upgraded to the latest API Tools in our build and got a confusing freeze report: http://download.eclipse.org/eclipse/downloads/drops4/N20160423-1500/apitools/freeze_report.html Several of these (for different bundles): REMOVED org.eclipse.ui.intro.universal#J2SE-1.4(Execution Environment) I know you said we have to exclude this, but didn't do so yet. I would rather expect CHANGED org.eclipse.ui.intro.universal#J2SE-1.4 to JavaSE-1.8 (Execution Environment) ADDED org.eclipse.jdt.ui.actions.WorkingSetFindAction This action exists since many years, hence clearly a false positive. If we also show compatible changes, we need to see this difference in the report, i.e. incompatible versus compatible changes. I've reverted the change with http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=f24b3fafdcfabe19dfa757a25b33ce6b9ca06c9c so that we can build a new version of the API Tools which we can then use in the build itself. If you have time to look at this, we can still put it into M7 or RC1. (In reply to Dani Megert from comment #8) > Several of these (for different bundles): > REMOVED org.eclipse.ui.intro.universal#J2SE-1.4(Execution Environment) > > I know you said we have to exclude this, but didn't do so yet. I would > rather expect > CHANGED org.eclipse.ui.intro.universal#J2SE-1.4 to JavaSE-1.8 (Execution > Environment) Or have a corresponding ADDED entry ADDED org.eclipse.ui.intro.universal#JavaSE-1.8 (Execution Environment) It looks like the change in DeltaXmlVisitor.processLeafDelta(IDelta) caused bug 492337: Instead of flags = 72 the entries have flags = DEPRECATION Let's see whether reverting also fixes bug 492337. I'll take a look at bug 492337. (In reply to Dani Megert from comment #9) > Or have a corresponding ADDED entry > ADDED org.eclipse.ui.intro.universal#JavaSE-1.8 (Execution Environment) This is what I have done. (In reply to Olivier Thomann from comment #12) > (In reply to Dani Megert from comment #9) > > Or have a corresponding ADDED entry > > ADDED org.eclipse.ui.intro.universal#JavaSE-1.8 (Execution Environment) > This is what I have done. You mean now? Currently it's not in the report as you can see. (In reply to Dani Megert from comment #13) > (In reply to Olivier Thomann from comment #12) > > (In reply to Dani Megert from comment #9) > > > Or have a corresponding ADDED entry > > > ADDED org.eclipse.ui.intro.universal#JavaSE-1.8 (Execution Environment) > > This is what I have done. > You mean now? Currently it's not in the report as you can see. I mean I fixed it :-) (In reply to Dani Megert from comment #8) > ADDED org.eclipse.jdt.ui.actions.WorkingSetFindAction The problem comes from the display of what has been found. The message from the delta is: "The method org.eclipse.jdt.ui.actions.WorkingSetFindAction.run(IJavaElement[]) has been added, overriding a method from a superclass" which is true. I am trying not to filter any delta and instead require a filter to be added for them. So in this case, I can filter it right way or I can improve the way the delta is reported requiring an exclusion to be added. Let me know what you prefer. (In reply to Olivier Thomann from comment #14) > (In reply to Dani Megert from comment #13) > > (In reply to Olivier Thomann from comment #12) > > > (In reply to Dani Megert from comment #9) > > > > Or have a corresponding ADDED entry > > > > ADDED org.eclipse.ui.intro.universal#JavaSE-1.8 (Execution Environment) > > > This is what I have done. > > You mean now? Currently it's not in the report as you can see. > I mean I fixed it :-) > > (In reply to Dani Megert from comment #8) > > ADDED org.eclipse.jdt.ui.actions.WorkingSetFindAction > The problem comes from the display of what has been found. > The message from the delta is: > "The method > org.eclipse.jdt.ui.actions.WorkingSetFindAction.run(IJavaElement[]) has been > added, overriding a method from a superclass" > which is true. I am trying not to filter any delta and instead require a > filter to be added for them. So in this case, I can filter it right way or I > can improve the way the delta is reported requiring an exclusion to be added. > Let me know what you prefer. The report should report the real issue which is the compatible change of adding WorkingSetFindAction.run(IJavaElement[]). The filter proposal should propose to filter that explicit thing. (In reply to Dani Megert from comment #15) > The report should report the real issue which is the compatible change of > adding WorkingSetFindAction.run(IJavaElement[]). It does report it. It is just that its display is bogus. I wonder if the best is not to report the actual delta's message which is very detailed. I'll make sure we provide a good way to filter it out in the exclude list. I think changing the BREE should be reported as a compatible change. Best as CHANGED. (In reply to Dani Megert from comment #17) > I think changing the BREE should be reported as a compatible change. Best as > CHANGED. CHANGED is fine when you go from one BREE to another one. When you have more than one, how do you know which one is changed? In this case reported REMOVED and ADDED is fine. (In reply to Olivier Thomann from comment #18) > (In reply to Dani Megert from comment #17) > > I think changing the BREE should be reported as a compatible change. Best as > > CHANGED. > CHANGED is fine when you go from one BREE to another one. When you have more > than one, how do you know which one is changed? In this case reported > REMOVED and ADDED is fine. Well, that's not the case for 'org.eclipse.ui.intro.universal' - it simply moved from 1.4 to 1.8. (In reply to Dani Megert from comment #19) > Well, that's not the case for 'org.eclipse.ui.intro.universal' - it simply > moved from 1.4 to 1.8. This is what I mean. When you go from one to one, I can report a CHANGED, but for all other cases it would be REMOVED and ADDED. For me this is a detail. As long as I see what has changed, both would be ok. Created attachment 261257 [details]
Proposed patch including updated regression tests
Daniel, Vikas, please try this patch. I could run a deprecation report and a freeze report successfully and all tests are green except the test for old style bundle that is also failing without the patch.
There are still some issues with the bundle org.eclipse.core.databinding.property, but this is due to a bogus .api_description file in the M6 zip. Some signatures are unresolved causing some bogus errors to be reported. (In reply to Olivier Thomann from comment #21) > Created attachment 261257 [details] [diff] > Proposed patch including updated regression tests > > Daniel, Vikas, please try this patch. I could run a deprecation report and a > freeze report successfully and all tests are green except the test for old > style bundle that is also failing without the patch. Hi Olivier, can you post the detailed steps here how you created the reports? If I recall correctly one has to go via Ant scripts and it's not possible via UI. Thanks! Will the freeze report show the difference between compatible and incompatible changes? This is the ant script I used to get the freeze report: <?xml version="1.0" encoding="UTF-8"?> <project name="api_freeze_reporting" default="run" basedir="."> <target name="run"> <apitooling.apifreeze debug="true" baseline="C:/api/eclipse-SDK-4.6M6-win32-x86_64/eclipse" profile="C:/api/eclipse-SDK-N20160423-1500-win32-x86_64/eclipse" report="C:/api/report_freeze.xml" excludeList="C:/api/exclude.list" /> <apitooling.apifreeze_reportconversion debug="true" htmlfile="C:/api/report_freeze.html" xmlfile="C:/api/report_freeze.xml" /> </target> </project> I don't remember about the way incompatible changes are shown compare to compatible changes. I modified the report to list the actual message from the delta which is more meaningful when you get something listed. Let me know what you think. (In reply to Olivier Thomann from comment #24) > I don't remember about the way incompatible changes are shown compare to > compatible changes. So far we only showed incompatible ones. I would expect to render them in either different sections or in different color. (In reply to Dani Megert from comment #25) > So far we only showed incompatible ones. I would expect to render them in > either different sections or in different color. No, so far we were reporting compatible and incompatible ones. I made changes to make sure we report incompatible ones with a red background color. (In reply to Olivier Thomann from comment #26) > (In reply to Dani Megert from comment #25) > > So far we only showed incompatible ones. I would expect to render them in > > either different sections or in different color. > No, so far we were reporting compatible and incompatible ones. Mh no, otherwise this bug would have been obsolete. (In reply to Dani Megert from comment #27) > Mh no, otherwise this bug would have been obsolete. I will attach a new report where you can see compatible and incompatible changes. We used to filter some of the compatible changes that we should not have filtered, but some compatible changes were already shown properly. Created attachment 261264 [details]
new report format
Created attachment 261265 [details]
Updated patch to put incompatible changes in red
(In reply to Olivier Thomann from comment #29) > Created attachment 261264 [details] > new report format The red color makes it hard to read the text and hurts my eyes ;-) (In reply to Dani Megert from comment #31) > (In reply to Olivier Thomann from comment #29) > > Created attachment 261264 [details] > > new report format > > The red color makes it hard to read the text and hurts my eyes ;-) What do you propose? It was the idea to hurt eyes as these ones should never happen. (In reply to Olivier Thomann from comment #32) > (In reply to Dani Megert from comment #31) > > (In reply to Olivier Thomann from comment #29) > > > Created attachment 261264 [details] > > > new report format > > > > The red color makes it hard to read the text and hurts my eyes ;-) > What do you propose? It was the idea to hurt eyes as these ones should never > happen. I would use bold and explain at the top that bold means breaking. Just to clarify: that example report does not use the exclude_list.txt: http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/tree/eclipse.platform.releng.tychoeclipsebuilder/eclipse/apiexclude/exclude_list.txt?h=master ? Created attachment 261404 [details]
Message not found in report_freeze.html
I get this with this patch
ADDED Message not found for id: 0
REMOVED Message not found for id: 0
delta.getFlags returns 68 which doesn't have a matching id. So 0 is returned. Also in IDelta, public static final int TYPE_ARGUMENT = 68; My suggestion would be white against red background
td.incompatible {
background-color: red;
color: white;
}
Since freeze report and deprecation report baseline will be no longer changed for 4.6, moving this to 4.7 Bulk move out of 4.8 target milestone. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. Please reopen if the problem still persists. |