Closed Bug 637034 Opened 9 years ago Closed 9 years ago

Add a native mkdir to pymake.builtins

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Mitch, Assigned: Mitch)

References

Details

(Whiteboard: fixed-in-bs)

Attachments

(1 file)

pymake has native, built-in implementations of "rm", "sleep" and "touch". We might as well have a native mkdir.

http://hg.mozilla.org/mozilla-central/file/151d55505c10/build/pymake/pymake/builtins.py
Attached patch PatchSplinter Review
This adds mkdir to pymake. Includes new mkdir tests plus makes the rm tests more thorough with subdirs.
Attachment #515364 - Flags: review?(khuey)
Comment on attachment 515364 [details] [diff] [review]
Patch

I'm going to pass this to ted since he's the python guy.

I also think we decided to use PEP 8 spacing (4 spaces) in the build system.
Attachment #515364 - Flags: review?(khuey) → review?(ted.mielczarek)
Comment on attachment 515364 [details] [diff] [review]
Patch

We did say PEP-8, but this is existing code that has 2-space indent. I don't know that it's worth having mismatched indents or reformatting all our code.

>diff --git a/build/pymake/pymake/builtins.py b/build/pymake/pymake/builtins.py
>--- a/build/pymake/pymake/builtins.py
>+++ b/build/pymake/pymake/builtins.py
 >+  parents = False
>+  for o, a in opts:
>+    if o in ('-p', '--parents'):
>+      parents = True
>+  for f in args:
>+    try:
>+      os.makedirs(f)

This doesn't strictly match the behavior of rm. If you did $(MKDIR) foo/bar, without -p, this code would still create foo/bar instead of failing. You should just use os.mkdir instead of makedirs if parents is False. You have a test for this in mkdir-fail.mk, but not for your builtin. :)

>+    except OSError, e:
>+      if (e.errno == errno.EEXIST) and (parents == True):

Some idiomatic Python nits: you don't need the parentheses, also, parents is a bool, so just saying "and parents:" is fine.

r=me with those fixes.
Attachment #515364 - Flags: review?(ted.mielczarek) → review+
Did you land this in bsmedberg's user repo as well?
http://hg.mozilla.org/mozilla-central/rev/c39ecf6d18ba
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 645356
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.