786891 - Add capability to auto-prefix modules to python script that imports css reftests

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scott Johnson (:jwir3)]

We should be able to automatically prefix some css modules when we import from an hg repository using dbaron's csswg import script. (If you're unfamiliar with this, what it does is copy individual css reftests from a local hg repository into our version of them, checked-in to the tree, with a log file saying what was updated and by which revision).

Note that this is not part of the build.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b786891

Added support for prefixing individual modules. Also, sorry, I got a little refactor-crazy... ;) I refactored a lot, including modularizing a bunch of the script into individual functions so it was more clear (at least to me) what part of the script was doing what.

I think most of the function names are self-explanatory, that's why I didn't add comments, but I can document all of them if you'd prefer.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 656692 [details] [diff] [review]
b786891

This mostly looks fine, but I think as far as what to prefix -- you should just have a list of substitutions to make rather than try to figure it out from the spec.  There will be points where we need to prefix some properties and not others from the same spec, and also times we'll want to prefix values.

Also, part of the intent here is that all the configuration of what we import is checked in to the tree.  Having a command line option for what to prefix is bad, since it makes it harder for somebody to just rerun the import to pull in updated tests.  Configuration (except for the path to the csswg repository) should all be in the script or in files that it reads in from the tree.

It would have been nice if the refactoring and the new features were separate patches (e.g., in a patch queue), but it's ok as-is, so don't bother trying to untangle it.

Also, stick to the 4-space indent, and put the imports at the top rather than inside prefix_occurrences_of.  And I think it's clearer to not import individual functions, because then the callsite shows the module (e.g., shutil.move at the callsite is clearer).

And you should be able to use the filehandle returned by mkstemp (the first half of the tuple) rather than calling open on the name to open it a second time.

Comment 3

[Scott Johnson (:jwir3)]

Attachment: b786891 (v2)

(In reply to David Baron [:dbaron] from comment #2)
> Comment on attachment 656692 [details] [diff] [review]
> b786891
> 
> This mostly looks fine, but I think as far as what to prefix -- you should
> just have a list of substitutions to make rather than try to figure it out
> from the spec.  There will be points where we need to prefix some properties
> and not others from the same spec, and also times we'll want to prefix
> values.

Ok, I've fixed this. It's too bad, though... I was proud of having thought of pulling them automatically. ;) I did consider the values as a possible hurdle, though, that we would need to overcome, and I thought about it for a while then decided, "This looks elegant, so I'm going to let a future user worry about the values problem." :)

> Also, part of the intent here is that all the configuration of what we
> import is checked in to the tree.  Having a command line option for what to
> prefix is bad, since it makes it harder for somebody to just rerun the
> import to pull in updated tests.  Configuration (except for the path to the
> csswg repository) should all be in the script or in files that it reads in
> from the tree.

Ok, I've removed the command line option. As another option, we could have added that to the log file, so that it would say, "Invoked with command: import-tests.py somepath -X blah....", but I agree with what you're saying - it should be as easy to run as possible, and as few configuration options as possible so that it can be reproduced exactly.

> It would have been nice if the refactoring and the new features were
> separate patches (e.g., in a patch queue), but it's ok as-is, so don't
> bother trying to untangle it.

Yeah, sorry about that. I originally did the refactoring as part of understanding what the script did, and reorganizing it locally. I didn't realize until I did an hg qref for the changes I wanted that those would be included as well. At that point, it seemed like they were reasonable changes, so I thought I would just leave them in.

> Also, stick to the 4-space indent, and put the imports at the top rather
> than inside prefix_occurrences_of.  And I think it's clearer to not import
> individual functions, because then the callsite shows the module (e.g.,
> shutil.move at the callsite is clearer).

Done.

> And you should be able to use the filehandle returned by mkstemp (the first
> half of the tuple) rather than calling open on the name to open it a second
> time.
Well, I don't think so, because this is an os-level filehandle, which doesn't have read/write. I use open() so that I can use read(), which is easier than the low-level I/O provided by os.open.

Comment 4

[Scott Johnson (:jwir3)]

Attachment: b786891 (v3)

Whoops, noticed a whitespace error with the previous patch.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 656711 [details] [diff] [review]
b786891 (v3)

>     shutil.copyfile(srcfile, destfile)
>+    prefix_occurrences_of(destfile, gPrefixedProperties)

It might make sense to remove the copy here, remove the temporary file business in the prefixing code, and just read from the source and write the prefixed stuff to dest.  (Then no more mkstemp, no more tempfile module, etc.)  And maybe name prefix_occurrences_of to copy_and_prefix()?

>+            hashloc = specurl.find("#")
>+            if hashloc == -1:
>+                trimmedspecurl = specurl
>+            else:
>+                trimmedspecurl = specurl[:hashloc]
>+

You don't need to add this anymore; it's now unused.

>+def setup_parser():
>+    global gParser, gArgs, gOptions
>+    gParser = OptionParser()
>+    gParser.usage = \
>+    '''%prog <clone of hg repository>
>+            Import reftests from a W3C hg repository clone. The location of
>+            the local clone of the hg repository must be given on the command
>+            line.'''
>+    (gOptions, gArgs) = gParser.parse_args()
>+    if len(gArgs) != 1:
>+        gParser.error("Too few arguments specified.")

Maybe call this read_options() instead of setup_parser()?  And the OptionParser doesn't need to be global anymore, so how about back to just "op"?


Also, why bother with prefixing of break-before, etc.?  We don't implement those break-* aliases yet, but when we do, I think we'll probably go straight to unprefixed, so I'd just skip prefixing them.


r=dbaron with that

Comment 6

[Scott Johnson (:jwir3)]

Fixed issues discussed in comment 5 and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/021eea9c166b

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh... but I forgot one review comment:

I meant to ask you to commit the imported tests in the same revision as the changes to the importing script.  Now it looks like the imported tests and the importing script are out-of-sync, which is bad.  (It means somebody else can't just run the import script to update things without also having other changes -- the ones you just made -- also take effect at the same time.)

In any case, could you commit the results as well?  (If there are failing tests you'll need to add a mechanism to the importing script to read a list of known failures for when it's generating the reftest.list.)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Alternatively, you could temporarily comment out the new directories you've added to the list of directories to import from.)

Comment 9

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #8)
> (Alternatively, you could temporarily comment out the new directories you've
> added to the list of directories to import from.)

Sure, I think this would be a better approach, since I intend to commit the new tests as part of another bug's patch. 

Do you think it's worth backing out and recommitting, or should another patch be ok?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think just commenting out the directories is ok; certainly no need to back out the whole patch.

That said, the idea when I wrote the script would be that it would read in a list of known failures (maybe from a "known-failures" file in the same directory as import-tests.py... probably listed by the destination path, since the source path is likely to move around due to the way the csswg repository is managed), and would mark those tests as failing when it generates the manifest (and give an error if something listed there isn't actually a test that it imports).  So if you want to do that, it would be great, since it would mean we're further down the path towards being able to import more tests.

Comment 11

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #10)
> I think just commenting out the directories is ok; certainly no need to back
> out the whole patch.

Done. New changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4d9e0663b5

> That said, the idea when I wrote the script would be that it would read in a
> list of known failures (maybe from a "known-failures" file in the same
> directory as import-tests.py... probably listed by the destination path,
> since the source path is likely to move around due to the way the csswg
> repository is managed), and would mark those tests as failing when it
> generates the manifest (and give an error if something listed there isn't
> actually a test that it imports).  So if you want to do that, it would be
> great, since it would mean we're further down the path towards being able to
> import more tests.

Sure, I'll work on this.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/021eea9c166b
https://hg.mozilla.org/mozilla-central/rev/fb4d9e0663b5