Last Comment Bug 767833 - pymake on Windows is busted with config/tests/makefiles/autodeps/check_mkdir.tpy
: pymake on Windows is busted with config/tests/makefiles/autodeps/check_mkdir.tpy
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks: 593585
  Show dependency treegraph
 
Reported: 2012-06-24 14:43 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-08-08 09:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (5.46 KB, patch)
2012-07-01 16:05 PDT, Siddharth Agarwal [:sid0] (inactive)
khuey: review+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2012-06-24 14:43:02 PDT
$ 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 :/
Comment 1 Siddharth Agarwal [:sid0] (inactive) 2012-07-01 16:05:56 PDT
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.
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.
Comment 2 Siddharth Agarwal [:sid0] (inactive) 2012-07-01 16:09:08 PDT
Hmm, maybe I shouldn't use path2posix on non-Windows platforms either.
Comment 3 Siddharth Agarwal [:sid0] (inactive) 2012-07-01 16:09:28 PDT
(What do you think, khuey?)
Comment 4 Joey Armstrong [:joey] 2012-07-02 06:58:15 PDT
(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
Comment 5 Siddharth Agarwal [:sid0] (inactive) 2012-07-02 07:09:57 PDT
(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.
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2012-07-02 07:10:40 PDT
> if they go through a bash shell involved somewhere

s/involved//
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-06 11:30:27 PDT
Can we just fix pymake?
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-06 11:30:35 PDT
Er, wrong bug.
Comment 9 Siddharth Agarwal [:sid0] (inactive) 2012-08-01 05:28:30 PDT
khuey: review ping?
Comment 10 Siddharth Agarwal [:sid0] (inactive) 2012-08-07 12:22:37 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/6947db9a3dcf
Comment 11 Ed Morley [:emorley] 2012-08-08 09:34:27 PDT
https://hg.mozilla.org/mozilla-central/rev/6947db9a3dcf

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