Closed Bug 1231315 Opened 4 years ago Closed 4 years ago

Build CONFIGURE_DEFINE_FILES at build time instead of during configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

No description provided.
Comment on attachment 8696936 [details] [diff] [review]
Build CONFIGURE_DEFINE_FILES at build time instead of during configure

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

::: python/mozbuild/mozbuild/action/process_define_files.py
@@ +29,5 @@
> +    with open(path, 'rU') as input:
> +        r = re.compile('^\s*#\s*(?P<cmd>[a-z]+)(?:\s+(?P<name>\S+)(?:\s+(?P<value>\S+))?)?', re.U)
> +        for l in input:
> +            m = r.match(l)
> +            if m:

The indent pyramid is hurting my eyes. Can you invert this and the "if name" below and use "continue."
Attachment #8696936 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 8696936 [details] [diff] [review]
> Build CONFIGURE_DEFINE_FILES at build time instead of during configure
> 
> Review of attachment 8696936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/action/process_define_files.py
> @@ +29,5 @@
> > +    with open(path, 'rU') as input:
> > +        r = re.compile('^\s*#\s*(?P<cmd>[a-z]+)(?:\s+(?P<name>\S+)(?:\s+(?P<value>\S+))?)?', re.U)
> > +        for l in input:
> > +            m = r.match(l)
> > +            if m:
> 
> The indent pyramid is hurting my eyes. Can you invert this and the "if name"
> below and use "continue."

I'm pretty sure you asked the same question when the same code was reviewed at the location I'm moving it from. And the problem is still the same: there's something happening after that if.
(In reply to Mike Hommey [:glandium] from comment #3)
> > The indent pyramid is hurting my eyes. Can you invert this and the "if name"
> > below and use "continue."
> 
> I'm pretty sure you asked the same question when the same code was reviewed
> at the location I'm moving it from. And the problem is still the same:
> there's something happening after that if.

Landed as is because the code is unchanged from what was already in the tree. If you feel strongly that this should be improved, please file a followup.
https://hg.mozilla.org/mozilla-central/rev/75e62a17dbba
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Blocks: 1234106
Depends on: 1235117
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.