Closed Bug 751865 Opened 8 years ago Closed 8 years ago

Preprocessor.py shouldn't apply filters to file names given on the command line

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla15

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 3 obsolete files)

With the change in bug 508942, all uses of do_include lead to filters being applied on the file name. But then, if you Preprocessor.py -fsubstitution -Dfoo=bar @foo@.in, Preprocessor.py tries to open bar.in instead of @foo@.in. It is a corner case, but I'm actually using Preprocessor.py this way for ease of Debian packaging.
Comment on attachment 621019 [details] [diff] [review]
Avoid Preprocessor.py applying filters to file names given on the command line

Review of attachment 621019 [details] [diff] [review]:
-----------------------------------------------------------------

This could use a unit test, but looks fine otherwise.
Attachment #621019 - Flags: review?(ted.mielczarek) → review+
Adds plenty of tests
Attachment #621395 - Flags: review?(ted.mielczarek)
Attachment #621019 - Attachment is obsolete: true
Comment on attachment 621395 [details] [diff] [review]
Avoid Preprocessor.py applying filters to file names given on the command line

Review of attachment 621395 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just one issue.

::: config/tests/unit-Preprocessor.py
@@ +22,5 @@
> +    f = open('foo', 'r')
> +
> +  will thus assign the NamedIO instance for the file 'foo' to f.
> +  """
> +  def __init__(self, files):

You could write this as **files so you can just pass them as separate args and not an actual list.

@@ +529,5 @@
> +  def test_undefined_variable(self):
> +    f = NamedIO("undefined_variable.in", """#filter substitution
> +@foo@
> +""")
> +    with self.assertRaises(Preprocessor.Error) as cm:

assertRaises as a context manager is unfortunately a Python 2.7 feature. :-(
Attachment #621395 - Flags: review?(ted.mielczarek) → review+
I think this covers the assertRaises as context manager cases.
Attachment #621502 - Flags: review?(ted.mielczarek)
Attachment #621395 - Attachment is obsolete: true
Man, python 2.5 is a PITA.
Attachment #621524 - Flags: review?(ted.mielczarek)
Attachment #621502 - Attachment is obsolete: true
Attachment #621502 - Flags: review?(ted.mielczarek)
Comment on attachment 621524 [details] [diff] [review]
Avoid Preprocessor.py applying filters to file names given on the command line

Review of attachment 621524 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/tests/unit-Preprocessor.py
@@ +560,5 @@
> +      self.pp.do_include(f)
> +    except Preprocessor.Error, exception:
> +      self.assertEqual(exception.key, 'FILE_NOT_FOUND')
> +    else:
> +      self.assertEqual(1, 0)

You can still use assertRaises, it's just sort of annoying. You'd have to write this like:
self.assertRaises(Preprocessor.Error, self.pp.do_include, f)
Attachment #621524 - Flags: review?(ted.mielczarek) → review+
http://hg.mozilla.org/mozilla-central/rev/b65d61a1d0ee
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.