Last Comment Bug 751865 - Preprocessor.py shouldn't apply filters to file names given on the command line
: Preprocessor.py shouldn't apply filters to file names given on the command line
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mike Hommey [:glandium]
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks: 508942
  Show dependency treegraph
 
Reported: 2012-05-04 05:36 PDT by Mike Hommey [:glandium]
Modified: 2012-05-07 17:52 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Avoid Preprocessor.py applying filters to file names given on the command line (2.65 KB, patch)
2012-05-04 05:37 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Avoid Preprocessor.py applying filters to file names given on the command line (6.20 KB, patch)
2012-05-06 03:03 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Avoid Preprocessor.py applying filters to file names given on the command line (6.30 KB, patch)
2012-05-06 22:22 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Avoid Preprocessor.py applying filters to file names given on the command line (9.00 KB, patch)
2012-05-07 00:39 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-05-04 05:36:22 PDT
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 1 Mike Hommey [:glandium] 2012-05-04 05:37:26 PDT
Created attachment 621019 [details] [diff] [review]
Avoid Preprocessor.py applying filters to file names given on the command line
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-05-04 08:33:23 PDT
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.
Comment 3 Mike Hommey [:glandium] 2012-05-06 03:03:39 PDT
Created attachment 621395 [details] [diff] [review]
Avoid Preprocessor.py applying filters to file names given on the command line

Adds plenty of tests
Comment 4 Ted Mielczarek [:ted.mielczarek] 2012-05-06 07:11:25 PDT
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. :-(
Comment 5 Mike Hommey [:glandium] 2012-05-06 22:22:35 PDT
Created attachment 621502 [details] [diff] [review]
Avoid Preprocessor.py applying filters to file names given on the command line

I think this covers the assertRaises as context manager cases.
Comment 6 Mike Hommey [:glandium] 2012-05-07 00:39:13 PDT
Created attachment 621524 [details] [diff] [review]
Avoid Preprocessor.py applying filters to file names given on the command line

Man, python 2.5 is a PITA.
Comment 7 Ted Mielczarek [:ted.mielczarek] 2012-05-07 05:02:37 PDT
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)
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-05-07 17:52:49 PDT
http://hg.mozilla.org/mozilla-central/rev/b65d61a1d0ee

Note You need to log in before you can comment on or make changes to this bug.