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

Increment the version in install.rdf when we mass sign the add-on (bug 1135139) #463

Merged

Conversation

magopian
Copy link
Contributor

@magopian magopian commented Mar 4, 2015

Fixes bug 1135139

Following several discussions with @kmaglione and @joernhees (from RDFLib/rdflib#468), I believe we can't reliably use rdflib in this case. I've thus given a try to lxml.etree.

Here's the diff on the two different install.rdf formats that I know of (left is the file I got from XPI files, the right is the result of the _bump_version_in_install_rdf method):

"standard" (most common) install.rdf:
screen shot 2015-03-09 at 14 29 18

"alternate" (deprecated?) install.rdf (the indentation is mine, the end result put each node on a single line):
screen shot 2015-03-09 at 14 29 02

@magopian magopian force-pushed the 1135139-increment-version-script-signing branch 3 times, most recently from a37b12d to 2e851c1 Compare March 9, 2015 13:30
@kmaglione
Copy link
Contributor

Why do you believe that rdflib won't work reliably here? I tested several install.rdf files, and with the fixes for the non-namespaced about attribute, the results were always correct.

@kmaglione
Copy link
Contributor

If we're going to go the lxml route, I think you're going about it the wrong way. You're going to miss Description elements with em:version attributes but no namespace for the about attribute (these are common). I'm not an expert on RDF XML, but I think there are some other valid formats it's also going to miss.

I think the best approach is to change any em:version attribute or node values, regardless of where they are. They have no official uses for anything other than specifying the extension version, so this should be fairly safe. This would suggest an xpath along the lines of //em:version | //@em:version, and just replacing them all.

@magopian
Copy link
Contributor Author

Regarding rdflib, i'm following the advice from @joernhees who said that even with the fix, there's this issue with the Bnode inserted, and thus the serialized result may be all broken. Haven't tried though.

I'll give it a try with your xpath, thanks a lot for the feedback!

@joernhees
Copy link

Not exactly, my rationale was mostly compatibility concerns...

The thing with rdflib here will be that you are using a very old version of RDF/XML as install.rdf template. The fixes make rdflib able to parse it again, but serializing will create a more up to date RDF/XML format. That output will be correct RDF/XML, but it will probably look quite different (order-wise as RDF is a graph, nesting-wise as the serializer prefers less nested elements, BNode-wise as the the serializer introduces local identifiers...).

As far as i understood you can't easily change the install.rdf template, much less all extensions that used it. Further, as was mentioned to me, many other things might depend on the current install.rdf (maybe with their own rdf parsers, xml parsers or even regexps). That's why in this "round-tripping" case with minimal changes to the version element i recommend an xml parser. This should allow you to only modify the one element you're after, leave the order and structure of the xml intact and have minimal / no impact on legacy code.

@kmaglione
Copy link
Contributor

Different format isn't a problem. Lots of extensions use machine-generated RDF XML in a similar format. As long as the data is the same, it's not an issue.

@magopian magopian force-pushed the 1135139-increment-version-script-signing branch from 2e851c1 to ae3b4d6 Compare March 10, 2015 09:46
@magopian
Copy link
Contributor Author

@kmaglione I made the code a bit more generic as you suggested.

# that has the "em:version" attribute if it's the alternate format.
node = tree.xpath(
'//em:version | //*[@em:version]',
namespaces={'em': 'http://www.mozilla.org/2004/em-rdf#'})[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd rather this be a for loop just in case there are extra em:version nodes.

@kmaglione
Copy link
Contributor

The new XML code looks good to me, aside from the minor comments. But the version number bumping code definitely needs to change.

We already have code for dealing with toolkit versions in validator.version. I think the easiest thing to do would be to make these objects mutable. I wrote that module, so let me know if you want me to do that.

@magopian magopian force-pushed the 1135139-increment-version-script-signing branch from ae3b4d6 to e7b04a6 Compare March 11, 2015 10:30
@magopian magopian force-pushed the 1135139-increment-version-script-signing branch from e7b04a6 to 162dc4e Compare March 25, 2015 17:31
@magopian
Copy link
Contributor Author

magopian commented Apr 2, 2015

According to https://bugzilla.mozilla.org/show_bug.cgi?id=1135139 we're good to merge on this one now?

@magopian magopian force-pushed the 1135139-increment-version-script-signing branch from 162dc4e to ef60d4a Compare April 2, 2015 11:53
@kmaglione
Copy link
Contributor

I think so. Everything looks good to me.

magopian added a commit that referenced this pull request Apr 3, 2015
…t-signing

Increment the version in install.rdf when we mass sign the add-on (bug 1135139)
@magopian magopian merged commit 78a206e into mozilla:master Apr 3, 2015
@magopian magopian deleted the 1135139-increment-version-script-signing branch April 3, 2015 20:09
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.

3 participants