Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/newJsonApiCheck into lp:ubuntu-ui-toolkit/staging
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Tim Peeters on 2015-05-20 | ||||
| Approved revision: | 1299 | ||||
| Merged at revision: | 1511 | ||||
| Proposed branch: | lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/newJsonApiCheck | ||||
| Merge into: | lp:ubuntu-ui-toolkit/staging | ||||
| Diff against target: |
4745 lines (+2521/-2131) 9 files modified
.bzrignore (+1/-0) components.api (+1380/-1890) debian/control (+1/-0) debian/ubuntu-ui-toolkit-autopilot.install (+1/-0) tests/apicheck/apicheck.cpp (+1106/-0) tests/apicheck/apicheck.pro (+12/-0) tests/qmlapicheck.py (+0/-218) tests/qmlapicheck.sh (+18/-19) tests/tests.pro (+2/-4) |
||||
| To merge this branch: | bzr merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/newJsonApiCheck | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| PS Jenkins bot | continuous-integration | Approve on 2015-05-20 | |
| Tim Peeters | 2014-09-24 | Approve on 2015-05-11 | |
|
Review via email:
|
|||
Commit Message
Implement new API tool based on qmlplugindump producing JSON
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1285
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1286
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1287
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1288
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1288
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1291
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1294
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Tim Peeters (tpeeters) wrote : | # |
+AbstractButton 1.0 AbstractButton 0.1 ActionItem
ok, nice, it has the different versions and its parent.
We can consider to remove reduntant information and make the formatting a bit more clear, for example like this:
AbstractButton 1.0 0.1: ActionItem
| Tim Peeters (tpeeters) wrote : | # |
^note that the inherited component may need a version too. It is not the case right now, but with zsombi's upcoming versioning tree split (and new subtheming), Page 1.0 will inherit PageTreeNode 1.0 and Page 1.3 inherits PageTreeNode 1.3. So we'd need (for example, I don't remember the details of the versions in this case):
Page 0.1 1.0 1.1 1.2: PageTreeNode 1.2
and
Page 1.3: PageTreeNode 1.3
Also note that I have a preference for listing the version numbers in increasing order. But I don't have arguments for that.
| Tim Peeters (tpeeters) wrote : | # |
All of the types of the properties of a component have versions too. So we could mess up there if we change those.
Luckily we recently agreed that in a certain version of the UITK, we use only components of the same version. That is not the case yet until zsombi's changes fort he versioning land.
| Tim Peeters (tpeeters) wrote : | # |
Palette QtObject
property PaletteValues normal
property PaletteValues selected
Palette 1.3 Palette 0.1 QtObject
property PaletteValues normal
property PaletteValues selected
What is this? Why is Palette listed twice?
We may need to include namespaces. What if we have Ubuntu.
Actually that is already happening here:
Header 1.0 Header 0.1 AppHeader
property string _for_autopilot
property var actions
property bool animate
property Object config
property Item contents
property color dividerColor
property Flickable flickable
function var show()
function var hide()
readonly property bool moving
property var pageStack
property color panelColor
property var tabsModel
property string title
property bool useDeprecatedTo
Header 1.0 Header 0.1 Item
property string text
Where the second Header is ListItems.Header. And the first one is actually marked as internal and deprecated with qdoc strings. I guess however that the api-checker does not take those into account and having the deprecated Header is an issue that is not relevant for this MR.
| Tim Peeters (tpeeters) wrote : | # |
> 1295. By Christian Dywan 2 hours ago
> Add apicheck to debian/
>
> It shouldn't be in the plugin so this is the second best place.
I'd say we don't install it at all. We build it and use it at build-time, but I don't see why we need to install it.
| Tim Peeters (tpeeters) wrote : | # |
Instead of listing namespaces in components.api, we could generate separate API files:
- ubuntu.
- ubuntu.
- ubuntu.
etc.
That way it will also (when checking the diff) immediately become clear whether we should pay extra attention to the API change (ubuntu.
To take it one step further, we could have a separate API file for each version of the components.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1295
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Christian Dywan (kalikiana) wrote : | # |
> +AbstractButton 1.0 AbstractButton 0.1 ActionItem
> ok, nice, it has the different versions and its parent.
> We can consider to remove reduntant information
> and make the formatting a bit more clear, for example like this:
> AbstractButton 1.0 0.1: ActionItem
The reason unlike staging I made left types separate is that they can differ, for instance with UbuntuShape, the old code however didn't cope with this given that its C++ handling was very improvised:
UbuntuShape 1.2 UbuntuShape 1.0 UbuntuShape 0.1 Shape 1.0 Shape 0.1 Item
> ^note that the inherited component may need a version too.
> It is not the case right now, but with zsombi's upcoming
> tree split (and new subtheming), Page 1.0 will inherit PageTreeNode 1.0
> and Page 1.3 inherits PageTreeNode 1.3. So we'd need
> (for example, I don't remember the details of the versions in this case):
> Page 0.1 1.0 1.1 1.2: PageTreeNode 1.2
> and
> Page 1.3: PageTreeNode 1.3
That's a very good point. I'll look into it.
> Also note that I have a preference for listing the version numbers
> in increasing order. But I don't have arguments for that.
I actually have reasons for the current order:
1. Sometimes you want to see which components were introduced in eg. 1.3 or double-check that a new component is indeed in the new version - having it at the very left makes that a bit easier
2. UbuntuShape used to be called Shape. If it started with Shape 0.1 it'd have to be sorted by S and be very surprising if you look for it under U.
Point #2 as well as the previous point of different names for the same class could potentially be resolved by listing them separately - but that would require duplicating their respective API and might be confusing.
> All of the types of the properties of a component have versions too.
> So we could mess up there if we change those.
> Luckily we recently agreed that in a certain version of the UITK,
> we use only components of the same version. That is not the case
> yet until zsombi's changes fort he versioning land.
True. It may be worth scanning for this even given our plans so that we can catch accidental mixing. I'd suggest this to be a follow-up.
> Palette QtObject
> property PaletteValues normal
> property PaletteValues selected
> Palette 1.3 Palette 0.1 QtObject
> property PaletteValues normal
> property PaletteValues selected
> What is this? Why is Palette listed twice?
> We may need to include namespaces. What if we have
> Ubuntu.
…
> Actually that is already happening here:
> Header 1.0 Header 0.1 AppHeader
> property string _for_autopilot
> property var actions
> property bool animate
> […]
> Header 1.0 Header 0.1 Item
> property string text
>
> Where the second Header is ListItems.Header. And the first one is
> actually marked as internal and deprecated with qdoc strings.
> I guess however that the api-checker does not take those
> into account and having the deprecated Header is an issue
> that is not relevant for this MR.
You hit the nail on the head. The old Header is public and the new code is only taking type information into account.
I suppose it would ...
| Christian Dywan (kalikiana) wrote : | # |
> Palette QtObject
> property PaletteValues normal
> property PaletteValues selected
> Palette 1.3 Palette 0.1 QtObject
> property PaletteValues normal
> property PaletteValues selected
> What is this? Why is Palette listed twice?
> We may need to include namespaces. What if we have
> Ubuntu.
I don't know to be quite honest. The version-less Palette is a C++ type from what I can infer, no idea where it's registered.
| Tim Peeters (tpeeters) wrote : | # |
2228 + qtquick1-5-dev,
2229 + qtquick1-
Why do we need qtquick1? That's old stuff.
| Tim Peeters (tpeeters) wrote : | # |
2243 === added file 'tests/
2244 --- tests/apicheck/
2245 +++ tests/apicheck/
2246 @@ -0,0 +1,1085 @@
2247 +/*
2248 + * Copyright (C) 2014 Digia Plc and/or its subsidiary(-ies).
2249 + * Copyright (C) 2015 Christian Dywan
How much of this is an existing tool from Digia? Just wondering if it is only their tool then maybe we should package it separately and not include the source here?
| Tim Peeters (tpeeters) wrote : | # |
2300 +void collectReachabl
It would be good to document what the function does and what its parameters are. Also for the other functions.
| Tim Peeters (tpeeters) wrote : | # |
> > +AbstractButton 1.0 AbstractButton 0.1 ActionItem
> > ok, nice, it has the different versions and its parent.
> > We can consider to remove reduntant information
> > and make the formatting a bit more clear, for example like this:
> > AbstractButton 1.0 0.1: ActionItem
>
> The reason unlike staging I made left types separate is that they can differ,
> for instance with UbuntuShape, the old code however didn't cope with this
> given that its C++ handling was very improvised:
>
> UbuntuShape 1.2 UbuntuShape 1.0 UbuntuShape 0.1 Shape 1.0 Shape 0.1 Item
We could consider them as separate things, so
UbuntuShape 1.2 1.0 0.1: Item
and
Shape 1.0 0.1: Item
| Tim Peeters (tpeeters) wrote : | # |
> > Also note that I have a preference for listing the version numbers
> > in increasing order. But I don't have arguments for that.
>
> I actually have reasons for the current order:
> 1. Sometimes you want to see which components were introduced in eg. 1.3 or
> double-check that a new component is indeed in the new version - having it at
> the very left makes that a bit easier
> 2. UbuntuShape used to be called Shape. If it started with Shape 0.1 it'd have
> to be sorted by S and be very surprising if you look for it under U.
>
> Point #2 as well as the previous point of different names for the same class
> could potentially be resolved by listing them separately - but that would
> require duplicating their respective API and might be confusing.
Okay, that makes sense.
> > All of the types of the properties of a component have versions too.
> > So we could mess up there if we change those.
> > Luckily we recently agreed that in a certain version of the UITK,
> > we use only components of the same version. That is not the case
> > yet until zsombi's changes fort he versioning land.
>
> True. It may be worth scanning for this even given our plans so that we can
> catch accidental mixing. I'd suggest this to be a follow-up.
Okay. Let's keep it in mind as a future addition.
| Tim Peeters (tpeeters) wrote : | # |
> > > 1295. By Christian Dywan 2 hours ago
> > > Add apicheck to debian/
> > >
> > > It shouldn't be in the plugin so this is the second best place.
>
> > I'd say we don't install it at all. We build it
> > and use it at build-time, but I don't see why we need to install it.
>
> We want other packages serialize and validate their API. Also Shop and
> development tools need to validate application code against frameworks
> available in targets.
Ok. It has nothing to do with autopilot though. Perhaps it should go in a separate package? Let's check with zoltan/mirv? Or we keep it here and give it its own package in the future after it developed a bit further.
> The next tool I'm looking into is a QML code validator that doesn't require
> targets (images, chroots, installed components) to be available, which would
> be highly impractical, but instead relies on apicheck output.
| Tim Peeters (tpeeters) wrote : | # |
> > Instead of listing namespaces in components.api, we could generate
> > separate API files:
> > - ubuntu.
> > - ubuntu.
> > - ubuntu.
> > etc.
> >
> > That way it will also (when checking the diff) immediately become clear
> > whether we should pay extra attention to the API change
> > (ubuntu.
> > ubuntu.
> > ubuntu.
> >
> > To take it one step further, we could have a separate API file
> > for each version of the components.
>
> If you take a look at the commit history you'll see that I had a per-namespace
> split at one point in this branch. One of the reasons I later decided to undo
> it is that it become tedious to read and move lots of files compared to
> skipping one components.api.
>
> Also validity of API is right now black and white, components that we don't
> guarantee stability for won't be included, if we did include them they would
> practically speaking get special treatment anyway as they effectively can
> block merge requests.
>
> I'd like to discuss this, however. I have rough thoughts on hiding the manual
> handling of components.
> alternative to diff to be able to quantify guarantees.
I think we need to at least list the namespaces, even if it is all in one components.api file. So we can make sections and list namespaces first:
Ubuntu.Components
=================
Page 1.2 1.0 0.1: PageTreeNode
Ubuntu.
=======
etc
or, per component list its name including the import?
Ubuntu.
| Tim Peeters (tpeeters) wrote : | # |
Still open besides my replies to your comments:
- Why is Palette listed twice?
- Formatting (e.g. Page 1.1 1.0 0.1: PageTreeNode)
- Version of inherited component
- Versions of properties (follow-up)
- How to deal with namespaces
- Packaging (currently apicheck is part of the UITK-AP package)
- Documentation in apicheck.cpp
| Christian Dywan (kalikiana) wrote : | # |
> 2248 + * Copyright (C) 2014 Digia Plc and/or its subsidiary(-ies).
> 2249 + * Copyright (C) 2015 Christian Dywan
>
> How much of this is an existing tool from Digia? Just wondering if it is
> only their tool then maybe we should package it separately and not
> include the source here?
As you can see I modified the source ;-) The code is based on qmlplugindump. Zsombi asked if it made sense to factor out the "changes" but at this point apicheck is almost a complete rewrite. I might actually check making a clean break with the remaining bits if that makes it clearer. That'll delay this branch yet again, though.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1296
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
| Christian Dywan (kalikiana) wrote : | # |
> - Why is Palette listed twice?
The Palette is a C++ type that I can't get QML type information for. That's true for all attached properties (cf. ULLayoutsAttached). I'm at a loss as to where Palette is exported in the code, maybe we can accept it as a limitation for now and if we hit anything similar in the future it'll be more obvious.
> - Formatting (e.g. Page 1.1 1.0 0.1: PageTreeNode)
Changed. In the case of UbuntuShape it now is
Ubuntu.
> - Version of inherited component
> - Versions of properties (follow-up)
I'd like to leave both of these for follow-up.
> - How to deal with namespaces
Prefixed class names with namespace; there's certain objects I cannot get a QML type for and ergo no namespace - classes can "leak" into other namespaces so I can't guess in this case either. Same underlying issue as with the Palette.
> - Packaging (currently apicheck is part of the UITK-AP package)
> - Documentation in apicheck.cpp
I added a some explanations to the usage output.
> 2228 + qtquick1-5-dev,
> 2229 + qtquick1-
>
> Why do we need qtquick1? That's old stuff.
Good catch, I removed it.
| Tim Peeters (tpeeters) wrote : | # |
> > - Why is Palette listed twice?
> The Palette is a C++ type that I can't get QML type information for. That's
> true for all attached properties (cf. ULLayoutsAttached). I'm at a loss as to
> where Palette is exported in the code, maybe we can accept it as a limitation
> for now and if we hit anything similar in the future it'll be more obvious.
We have a Palette.qml in Ubuntu.
> > - Formatting (e.g. Page 1.1 1.0 0.1: PageTreeNode)
> Changed. In the case of UbuntuShape it now is
> Ubuntu.
>
> > - Version of inherited component
> > - Versions of properties (follow-up)
>
> I'd like to leave both of these for follow-up.
I think the version of inherited component is quite important. We don't have to solve everything in this MR because then it will never land, but let's report the bug when we land this MR, and see if we can put it on the backlog.
> > - How to deal with namespaces
> Prefixed class names with namespace; there's certain objects I cannot get a
> QML type for and ergo no namespace - classes can "leak" into other namespaces
> so I can't guess in this case either. Same underlying issue as with the
> Palette.
Ok. Those issues were present in the previous version of the API checker as well, so it is ok to leave it unsolved here.
> > - Packaging (currently apicheck is part of the UITK-AP package)
Do we want to keep it in the autopilot package?
> > - Documentation in apicheck.cpp
> I added a some explanations to the usage output.
>
> > 2228 + qtquick1-5-dev,
> > 2229 + qtquick1-
> >
> > Why do we need qtquick1? That's old stuff.
>
> Good catch, I removed it.
OK.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1298
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
UNSTABLE: http://
deb: http://
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
deb: http://
UNSTABLE: http://
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
- 1299. By Christian Dywan on 2015-05-20
FAILED: Continuous integration, rev:1284
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://