Closed
Bug 767833
Opened 13 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)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla17
People
(Reporter: rain1, Assigned: rain1)
References
Details
Attachments
(1 file)
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
$ 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 :/
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
Hmm, maybe I shouldn't use path2posix on non-Windows platforms either.
Assignee | ||
Comment 3•12 years ago
|
||
(What do you think, khuey?)
Comment 4•12 years ago
|
||
(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
Assignee | ||
Comment 5•12 years ago
|
||
(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.
Assignee | ||
Comment 6•12 years ago
|
||
> if they go through a bash shell involved somewhere
s/involved//
Can we just fix pymake?
Er, wrong bug.
Updated•12 years ago
|
Attachment #638235 -
Flags: feedback?(joey)
Assignee | ||
Comment 9•12 years ago
|
||
khuey: review ping?
Attachment #638235 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•