Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve forward-version/migration checks #234

Merged
merged 1 commit into from
May 2, 2020
Merged

Conversation

enen92
Copy link
Member

@enen92 enen92 commented Apr 28, 2020

Unfortunately the last approach do not work for all addons, causing problems in the repo-scrappers repository. The repository generator places clones of the same addon on upper branches if their dependencies are abi backwards compatible.
Means that if you push a xml scrapper to gotham branch, the same version will also be available in matrix branch (I mean in the addons.xml.gz and not in github). Addon-checker errors out because it sees the matrix addon as non-migration compatible (same addon version). This happens because we are blindly making that decision on the fact xbmc.python is not forward compatible between target and matrix - but the addon might not depend on xbmc.python at all.

So this PR changes the check to make it more generic, it looks over all the xbmc api dependencies (xbmc.python, xbmc.gui, xbmc.metadata, etc) and checks if they are forward compatible between the repo to which the addon is being submitted and the "upper branch" that also contains the addon (and just if the addon actually depend on any of them).

@enen92 enen92 requested a review from razzeee April 28, 2020 12:20
@enen92 enen92 force-pushed the master branch 2 times, most recently from f5a5dcd to 5ffecf4 Compare April 28, 2020 12:39
}
},
'xbmc.gui': {
'gotham': {'min_compatible': '5.0.0', 'advised': '5.0.0'},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if it would make sense to default to AddonVersion objects here? So that we don't need to parse these for every useage?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can disable checks I suppose. Hence this may not even be used. I think it's a good a idea but out of the scope of this PR

on xbmc abi dependencies and they are not forward compatible
@razzeee
Copy link
Member

razzeee commented Apr 29, 2020

Should we add the scrapers repo to the tests?

@razzeee
Copy link
Member

razzeee commented Apr 29, 2020

This seems buggy. https://travis-ci.org/github/xbmc/addon-check/jobs/681138217 errors for script.module.actionhandler but I'm not seeing the problem it suggests in the repo. https://github.com/xbmc/repo-scripts/tree/matrix

Not sure if I just misunderstood.

@enen92
Copy link
Member Author

enen92 commented Apr 29, 2020

That's odd, was it the only one you found? For some reason the addons.xml.gz from the repo lists that script.module in matrix?

@enen92
Copy link
Member Author

enen92 commented Apr 29, 2020

I see the problem: https://mirrors.kodi.tv/addons/matrix/script.module.actionhandler/

For some reason this addon doesn't have any xbmc.python dependency:

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<addon id="script.module.actionhandler" name="ActionHandler" version="1.0.3" provider-name="Philipp Temminghoff (phil65)">
	<extension point="xbmc.python.module" library="lib" />
	<extension point="xbmc.addon.metadata">
		<summary lang="en">Helper module for Event handling in WindowXMLs</summary>
		<description lang="en">Helper module to allow mapping Actions, Onfocus and click events via decorators</description>
		<platform>all</platform>
		<license>GNU GENERAL PUBLIC LICENSE Version 2.1, February 1999</license>
		<source>https://github.com/phil65/script.module.actionhandler</source>
	</extension>
</addon>

The problem is that the addon in jarvis doesn't have any dependency on xbmc.python and that made it propagate all the way till matrix:

https://github.com/xbmc/repo-scripts/blob/jarvis/script.module.actionhandler/addon.xml

This should be fixed, but it's not related to this PR

@enen92
Copy link
Member Author

enen92 commented Apr 29, 2020

Repo-scrappers should be added to the tests since rechi configured it there

@razzeee
Copy link
Member

razzeee commented Apr 29, 2020

That's odd, was it the only one you found?

I basically clicked on random through some of the test runs and only found that. There might be more.

@enen92
Copy link
Member Author

enen92 commented Apr 29, 2020

I'll take a look over the logs to find more references of forward abi compatible dependencies tomorrow. Although I think it will actually help us find similar problems to the one above

@razzeee
Copy link
Member

razzeee commented Apr 29, 2020

Agreed, still good to make sure there is no wrong behavior that we can see.

@enen92
Copy link
Member Author

enen92 commented May 1, 2020

@razzeee looked into the logs and found 2 addons broken in matrix (pr's to solve it are above ^^). Problem is that those addons did not define xbmc.python as a requirement and that made them being included in the matrix branch. I'd say this is good to go.

@razzeee razzeee merged commit 0d1a4a5 into xbmc:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants