jwir3 / checkiid Goto Github PK

View Code? Open in Web Editor NEW

2.0 2.0 1.0 359 KB

Script for checking interfaces for corresponding IID changes in mozilla code

Python 100.00%

checkiid's People

Contributors

Stargazers

Watchers

Forkers

sylvestre

checkiid's Issues

Script should be able to pull its own diff and operate on this

Right now, checkiid.py operates on an input (from stdin) of a diff file. It should, in addition, construct this diff file and run on it. This way, we can use it as a standalone tool, as well as an hg hook.

This should probably be used in combination with a set of revisions switches, so that we can use it incrementally.

areDescriptorsInLineAffectingBinaryCompat does not return correct values

The function IDLDescriptor.areDescriptorsInLineAffectingBinaryCompat() checks:

If there is at least one IDLDescriptor in a given line, and
If any descriptors in kDescriptorList affect binary compatibility.

What it should be checking is:

If there is at least one IDLDescriptor in a given line, and
Whether those descriptors affect binary compatibility.

Pull classes out into separate files

Currently, all the python classes are in a single file, checkiid.py. We should pull separate classes out into separate modules, to better modularize the code.

False positive on comment changes

checkiid is triggering an error on comment changes. For example, on this changeset:
http://hg.mozilla.org/mozilla-central/rev/feded4c79308

Maybe it should ignore line starting with:
^+[:space:]*

Script should ignore changes to dictionaries

The following incorrectly flags nsIDOMDeviceProximityEvent as needing an IID change:

diff --git a/dom/interfaces/events/nsIDOMDeviceProximityEvent.idl b/dom/interfaces/events/nsIDOMDeviceProximityEvent.idl
--- a/dom/interfaces/events/nsIDOMDeviceProximityEvent.idl                      
+++ b/dom/interfaces/events/nsIDOMDeviceProximityEvent.idl                      
@@ -16,12 +16,12 @@ interface nsIDOMDeviceProximityEvent : n                    
   readonly attribute double value;                                             
   readonly attribute double min;                                               
   readonly attribute double max;                                               
 };                                                                             


 dictionary DeviceProximityEventInit : EventInit                                
 {                                                                              
-   double value;                                                               
-   double min;                                                                 
-   double max;                                                                 
+   double value = Infinity;                                                    
+   double min = -Infinity;                                                     
+   double max = Infinity;                                                      
 };

Documentation is needed

We currently have a readme file, generated by github, in the root of the project directory, but it would be nice if we could add some documentation to this script, both in the readme file, and on the mozilla wiki, that details how to use the script.

Add ANSI color output to warnings/errors on output

It would be nice if the output of the script was easier to read by adding colors.

(CRASH) Script crashes in findAllSpecialBlocksForFile when file not found

If a file is not found the script will crash inside of findAllSpecialBlocksForFile. We should add an extra try block so that this error condition fails gracefully instead of crashing.

Example with pipe is failing

From the README.md file:
$ cat /tmp/firefox.diff | python /path/to/checkiid.py .
usage: checkiid.py [-h] [-V] [-d] [-t ] [-n]
repo [inputfile]
checkiid.py: error: too few arguments

but
$ python /path/to/checkiid.py repo /tmp/firefox.diff
seems to work

Consecutive unit tests do not overwrite the output file

If you run one unit test, then another one, and they don't have the same output, then the /tmp/ file is not overwritten. We should probably use an actual temp file, rather than a manually generated one (that's the same every time), and even probably delete it after we're done using it.

Script reports error when interface is renamed

When a diff of the following nature is found:

diff --git a/content/base/public/nsIMessageManager.idl b/content/base/public/nsIMessageManager.idl
--- a/content/base/public/nsIMessageManager.idl                                 
+++ b/content/base/public/nsIMessageManager.idl                                 
@@ -331,18 +331,18 @@ interface nsIFrameScriptLoader : nsISupp                  
   void loadFrameScript(in AString aURL, in boolean aAllowDelayedLoad);         

   /**                                                                          
    * Removes aURL from the list of scripts which support delayed load.         
    */                                                                          
   void removeDelayedFrameScript(in AString aURL);                              
 };                                                                             

-[scriptable, builtinclass, uuid(5f552699-01a2-4f17-833b-ddb3fa0d98b2)]         
-interface nsIPermissionChecker : nsISupports                                   
+[scriptable, builtinclass, uuid(134ccbf0-5c08-11e2-bcfd-0800200c9a66)]         
+interface nsIProcessChecker : nsISupports                                      
 {                                                                              

   /**                                                                          
    * Return true iff the "remote" process has |aPermission|.  This is          
    * intended to be used by JS implementations of cross-process DOM            
    * APIs, like so                                                             
    *                                                                           
    *   recvFooRequest: function(message) {                                     
@@ -356,9 +356,32 @@ interface nsIPermissionChecker : nsISupp                   
    * then applying this permission check doesn't add any security,             
    * though it doesn't hurt anything either.                                   
    *                                                                           
    * Note: If the remote content process does *not* have |aPermission|,        
    * it will be killed as a precaution.                                        
    */                                                                          
   boolean assertPermission(in DOMString aPermission);                          

+  /**                                                                          
+   * Return true iff the "remote" process has |aManifestURL|.  This is         
+   * intended to be used by JS implementations of cross-process DOM            
+   * APIs, like so                                                             
+   *                                                                           
+   *   recvFooRequest: function(message) {                                     
+   *     if (!message.target.assertContainApp("foo")) {                        
+   *       return false;                                                       
+   *     }                                                                     
+   *     // service foo request                                                
+   *                                                                           
+   * This interface only returns meaningful data when our content is           
+   * in a separate process.  If it shares the same OS process as us,           
+   * then applying this manifest URL check doesn't add any security,           
+   * though it doesn't hurt anything either.                                                                                                           
+   *                                                                           
+   * Note: If the remote content process does *not* contain |aManifestURL|,    
+   * it will be killed as a precaution.                                        
+   */                                                                          
+  boolean assertContainApp(in DOMString aManifestURL);                         
+                                                                               
+  boolean assertAppHasPermission(in DOMString aPermission);                    
+                                                                               
 };

The script thinks that 'nsIPermissionChecker' needs a new IID, because of the context line:

@@ -356,9 +356,32 @@ interface nsIPermissionChecker : nsISupp

We should cache a renamed interface so this doesn't confuse the script.

Wrong warnings displayed by checkiid

Running checkiid on the new beta (31), I am getting plenty of warnings:

WARNING: 'nsIDocumentEncoder.idl' was not found in local repository. Are you sure your repository is at the correct revision?

For this file, its path hasn't change for a while (at least 2008).
Path + patch look good:

$ find . -iname nsIDocumentEncoder.idl
./content/base/public/nsIDocumentEncoder.idl
$ grep nsIDocumentEncoder.idl /tmp/firefox.diff 
diff --git a/content/base/public/nsIDocumentEncoder.idl b/content/base/public/nsIDocumentEncoder.idl
--- a/content/base/public/nsIDocumentEncoder.idl
+++ b/content/base/public/nsIDocumentEncoder.idl

Failure on things that have code and comments on a single line

If we have input like the following (i.e. in the IDL file, not the diff):

If we have something like the following:
    attribute wstring plexName;      /* name of plex mode (like "simplex", "duplex",
                                      * "tumble" and various custom values) */

checkiid.py will fail to construct a correct block comment for this, because the first line doesn't //begin// with a comment block. We're going to have to figure out a way to get it to recognize characters, but only in specific cases.

Encapsulate script into a class

The checkiid module should be better encapsulated into a class. This will enable us to better represent it as a state machine (which it already is), but will allow us to keep "context" where the state is more understandable.

Script sometimes gets confused by full removal

When the diff file has input like the following:

diff --git a/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl b/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl
--- a/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl                  
+++ b/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl                  
@@ -1,32 +1,15 @@                                                               
 /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public         
  * License, v. 2.0. If a copy of the MPL was not distributed with this         
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */                 

 #include "nsISupports.idl"                                                     

-[scriptable, uuid(bbb20a59-524e-4662-981e-5e142814b20c)]                       
-interface nsIDOMCanvasGradient : nsISupports                                   
-{                                                                              
-  void addColorStop(in float offset, in DOMString color);                      
-};                                                                             
-                                                                               
-[scriptable, uuid(21dea65c-5c08-4eb1-ac82-81fe95be77b8)]                       
-interface nsIDOMCanvasPattern : nsISupports                                    
-{                                                                              
-};                                                                             
-                                                                               
-[scriptable, uuid(2d01715c-ec7d-424a-ab85-e0fd70c8665c)]                       
-interface nsIDOMTextMetrics : nsISupports                                      
-{                                                                              
-  readonly attribute float width;                                              
-};                                                                             
-

The script incorrectly reports these interfaces as needing IID changes. Any time an interface has been completely removed from a file, the script should identify this as not needing an IID change.

IDL checker script does not correctly install sub-modules

During installation, the setup.py script should be installing idlutils and prettyprinter to a system python directory (site-packages), but, at least on mac, it's not doing this properly.

Create a more robust test suite

There are inflexibilities with the current test suite. Namely, each test is based off of a specific revision range (as well as tree), and it's likely that, at the rate these are changing, the tests will become invalid very quickly.

We should setup some method of checking the commits on the given hg repo (and probably that the tree is the same tree), and aborting the test if we don't have the correct commits.

Unit test mode should ignore comments in ref files

If a .ref file has comments (e.g. lines prefixed by '#'), these should be ignored when testing for equivalence in the file and the output from the script.

Have the script checkout the hg revision end automatically

One of the issues with the current system is that the script expects that the hg repository given is at the same revision as the specified end revision. We should make sure that this is the case, since this can be done programmatically, and then re-checkout the revision that the user had prior.

This will likely eliminate some potential future confusion.