Closed Bug 767833 Opened 12 years ago Closed 12 years ago

pymake on Windows is busted with config/tests/makefiles/autodeps/check_mkdir.tpy

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: rain1, Assigned: rain1)

References

Details

Attachments

(1 file)

$ pymake check
make.py[0]: Entering directory 'c:\Users\Sid\mozilla\tbto\mozilla\config\tests\makefiles\autodeps'
Usage: make.py [options]

make.py: error: no such option: -E
c:\Users\Sid\mozilla\tbto\mozilla\config\tests\makefiles\autodeps\Makefile:40:0$ MAKECMD=c:/Users/Sid/mozilla-build/python/python.exe c:/Users/Sid/mozilla/mozilla-central/build/pymake/pymake/../make.py c:/Users/Sid/mozilla/tbto/mozilla/_virtualenv/Scripts/python.exe -E ../../../../../../comm-central/mozilla/config/tests/makefiles/autodeps/check_mkdir.tpy
c:\Users\Sid\mozilla\tbto\mozilla\config\tests\makefiles\autodeps\Makefile:40:0: command 'MAKECMD=c:/Users/Sid/mozilla-build/python/python.exe c:/Users/Sid/mozilla/mozilla-central/build/pymake/pymake/../make.py c:/Users/Sid/mozilla/tbto/mozilla/_virtualenv/Scripts/python.exe -E ../../../../../../comm-central/mozilla/config/tests/makefiles/autodeps/check_mkdir.tpy' failed, return code 2

The reason seems to be that at https://mxr.mozilla.org/mozilla-central/source/config/tests/makefiles/autodeps/Makefile.in#40, $(MAKE) has a space in it. I tried adding quotes but that caused the path to be mangled by msys :/
Blocks: 593585
Attached patch fix v1Splinter Review
Lots of fixes here.

1. Export MAKE and .PYMAKE directly into the child process's environment so that the issue I described in comment 0 doesn't occur.
2. split takes the maximum number of splits, not the maximum number of components, so the correct value there is 1.
3. Splitting then joining is broken because of the comment mentioned at http://docs.python.org/library/os.path.html?highlight=os.path.join#os.path.join, so just call dirname 5 times. I think it's pretty clear, but I can write it in a less functional style if you like :)
4. Don't pass in MSYS paths to pymake.
5. Pass in relative paths to make to bypass the MSYS vs Win32 path issue entirely. I don't really see the need for absolute paths there, and I was running into \-escaping issues. Sigh.

Joey, feedback would be great if I broke anything in your patch.
Assignee: nobody → sagarwal
Status: NEW → ASSIGNED
Attachment #638235 - Flags: review?(khuey)
Attachment #638235 - Flags: feedback?(joey)
Hmm, maybe I shouldn't use path2posix on non-Windows platforms either.
(What do you think, khuey?)
(In reply to Siddharth Agarwal [:sid0] from comment #1)
> Created attachment 638235 [details] [diff] [review]
> fix v1
> 
> Lots of fixes here.
> 
> 1. Export MAKE and .PYMAKE directly into the child process's environment so
> that the issue I described in comment 0 doesn't occur.

Ok this edit will break one of the testing abilities.  MAKECMD= was passed/used in place of $(MAKE) to support verifying both commands during a testing run.  If $(MAKE) or .PYMAKE are explicitly used only functionality of one command can be verified.

ps>  make.py: error: no such option: -E

This problem was created by removing the MAKECMD= assignment.


> 2. split takes the maximum number of splits, not the maximum number of components, so the correct value there is 1.

Good catch.  Excess split values will be discarded so not a big problem but split now matches the number of retained values.

> 3. Splitting then joining is broken because of the comment mentioned at
> http://docs.python.org/library/os.path.html?highlight=os.path.join#os.path.
> join, so just call dirname 5 times. I think it's pretty clear, but I can
> write it in a less functional style if you like :)

What was causing a problem here ?  posixpath() is able to handle arbitrary paths, containing drive letters, and will normalize slashes.  Is there an example case where this logic was not working ?  The test suite handled most path permutations so if something is not working properly another test case should be added.

ps>  My preference is usually to not hardcode values like (dirname x 5).  Logic which makes assumptions about path structure or values will eventually have problems when sources are relocated.


> 4. Don't pass in MSYS paths to pymake.

Not being facetious here, why not ?

Passing arbitrary paths into make is a fairly common op.  Override paths to test new/different commands, using pre-built dependencies to short-circuit and test logic, etc.  If paths cannot be handled properly more bugs will need to be opened to support proper behavior.


> 5. Pass in relative paths to make to bypass the MSYS vs Win32 path issue
> entirely. I don't really see the need for absolute paths there, and I was
> running into \-escaping issues. Sigh.

I do not think this is a good avenue to pursue WRT testing.  This test was explicitly written to detect known path problems - that will manifest within the makefiles.

At a minimum people should have the ability to invoke commands like these:
   $(MAKE) FOOBAR=`pwd`/myfoobar.sh
   $(MAKE) VERIFY=/bin/true

Massaging test data by passing in relative paths basically will ignore a larger problem space of having to deal with different path prefixes.  The common ones being:
   /foo/bar
   //foo/bar
   c:\bar
(In reply to Joey Armstrong [:joey] from comment #4)
> (In reply to Siddharth Agarwal [:sid0] from comment #1)
> > Created attachment 638235 [details] [diff] [review]
> > fix v1
> > 
> > Lots of fixes here.
> > 
> > 1. Export MAKE and .PYMAKE directly into the child process's environment so
> > that the issue I described in comment 0 doesn't occur.
> 
> Ok this edit will break one of the testing abilities.  MAKECMD= was
> passed/used in place of $(MAKE) to support verifying both commands during a
> testing run.  If $(MAKE) or .PYMAKE are explicitly used only functionality
> of one command can be verified.

Where is this verification of both commands taking place? I tried looking for it but couldn't find it.
> 
> Good catch.  Excess split values will be discarded so not a big problem but
> split now matches the number of retained values.

No, they won't be discarded. "a:b:c".split(":", 1) returns ["a", "b:c"].

> 
> > 3. Splitting then joining is broken because of the comment mentioned at
> > http://docs.python.org/library/os.path.html?highlight=os.path.join#os.path.
> > join, so just call dirname 5 times. I think it's pretty clear, but I can
> > write it in a less functional style if you like :)
> 
> What was causing a problem here ?  posixpath() is able to handle arbitrary
> paths, containing drive letters, and will normalize slashes.  Is there an
> example case where this logic was not working ?

"c:\\foo".split("\\") returns ["c:", "foo"].
os.path.join("c:", "foo") returns "c:foo" and not "c:\\foo". The two are different, as highlighted in the link I posted. dirname is the only safe way to deal with this. This doesn't have anything to do with path2posix though -- that's fine.

> ps>  My preference is usually to not hardcode values like (dirname x 5). 
> Logic which makes assumptions about path structure or values will eventually
> have problems when sources are relocated.

The original code hardcoded 5 too. I'm not making things worse here.

> > 4. Don't pass in MSYS paths to pymake.
> 
> Not being facetious here, why not ?

Because Pymake works with native Windows paths, not MSYS paths. See bug 770165 comment 1.

> I do not think this is a good avenue to pursue WRT testing.  This test was
> explicitly written to detect known path problems - that will manifest within
> the makefiles.
> 
> At a minimum people should have the ability to invoke commands like these:
>    $(MAKE) FOOBAR=`pwd`/myfoobar.sh
>    $(MAKE) VERIFY=/bin/true

The interaction between MSYS paths and Pymake is actually fairly complex. Relative paths typically work just fine but absolute ones are only converted from MSYS to Win32 if they go through a bash shell involved somewhere. In your examples I don't think it's a problem since Pymake will invoke bash for both of them. However the test will invoke python.exe directly, so no bash shell involved.
> if they go through a bash shell involved somewhere

s/involved//
Attachment #638235 - Flags: feedback?(joey)
khuey: review ping?
https://hg.mozilla.org/mozilla-central/rev/6947db9a3dcf
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: