Issue1720897
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2007-05-17 18:13 by draghuram, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
1668596_test.tar.gz | draghuram, 2007-05-17 18:17 | test files (not a patch) | ||
buildpy_patch.diff | draghuram, 2007-05-30 20:50 | comment added to test case |
Messages (15) | |||
---|---|---|---|
msg52639 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-17 18:13 | |
This patch fixes http://www.python.org/sf/1668596. If package_dir is '', there is no need to remove package_dir part from data file path names. I am also attaching 1668596_test.tar.gz. This has the directory I used to test the patch (both on Linux and Windows XP). I will need some more time to come up with a real test case that can be submitted as patch. I am hoping that if there is some one out there who is familiar with distutils test setup, they may be able to come up with a test case quicker than me. |
|||
msg52640 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-17 18:17 | |
File Added: 1668596_test.tar.gz |
|||
msg52641 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-17 18:25 | |
File Added: build_py.diff |
|||
msg52642 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-21 18:22 | |
File Added: buildpy_patch.diff |
|||
msg52643 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-21 18:24 | |
I managed to create a test case (is part of the patch now). I ran it on Linux and Windows XP and in both cases, it properly tests the problem. |
|||
msg52644 - (view) | Author: PJ Eby (pje) * | Date: 2007-05-30 16:31 | |
+1 on this approach; recommend commit of this patch (note, however, that the parens in "if (src_dir):" are redundant). |
|||
msg52645 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-30 17:19 | |
Thanks for reviewing. As for parens, they don't exist in the current patch. Right? I removed them quite some time back. |
|||
msg52646 - (view) | Author: Rodrigo Bernardo Pimentel (errebepe) | Date: 2007-05-30 17:58 | |
+1 on the fix. I do have a few minor suggestions, however. In the fix itself, the parens in "(src_dir)" are still there. If the fix will only be applied in >= 2.5, you might also use the "plen = len(src_dir)+1 if src_dir else 0" form. As for the test, since you're testing a very specific error scenario (Distribution.run_commands raising DistutilsFileError), I'd suggest catching the exception and using "self.fail", so that testing the unfixed code fails instead of issues an error. Since you expect the error to occur inside run_commands, I'd put the try...except block around the "dist.run_commands()" only, as in: # Auxiliary variable, since you need cleanup and self.fail exits before "finally": error = False try: ....dist.run_commands() except DistutilsFileError: ....error = True finally: ....os.chdir(cwd) if error: ....self.fail("run_commands fails with no package_dir") Gustavo Niemeyer also mentioned you should add a dosctring to the test explaining what exactly it's testing. To sum it up, I recommend commit of this patch regardless of my comments above, but I hope they're taken into consideration :) |
|||
msg52647 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-30 19:00 | |
errebepe, Thanks for the comments. I incorporated your suggestions about the test case. It will now check explicitly for DistutilsFileError and calls self.fail() if it gets this exception. I removed parens in "if (src_dir)" as well. I considered "plen = len(src_dir)+1 if src_dir else 0" form but I found it difficult to follow compared to an explicit "if". As to adding a docstring to the test case, I vaguely remember seeing some comment about not having docstrings in test functions. I also don't find docstrings in any of the test functions (found with a quick look at less test_*.py output). |
|||
msg52648 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-30 19:01 | |
File Added: buildpy_patch.diff |
|||
msg52649 - (view) | Author: Rodrigo Bernardo Pimentel (errebepe) | Date: 2007-05-30 19:42 | |
True, I couldn't find many tests with docstrings either (though there are a few: find -name test_\*.py -exec egrep -nHB1 '^\s*"""' {} \; | grep -C1 'def test_' | less). On the other hand, most test names are somewhat descriptive of what they are testing. I think a brief description, either in a docstring or in a regular comment (which are common in existing tests), would be helpful to anyone reading it later. As for the rest, great! Thanks for the patch and for the quick response :) |
|||
msg52650 - (view) | Author: Raghuram Devarakonda (draghuram) | Date: 2007-05-30 20:50 | |
errebepe, I added a comment to the test case. I need some more time to analyze your find command but that is not relevant to this patch :-). File Added: buildpy_patch.diff |
|||
msg52651 - (view) | Author: Rodrigo Bernardo Pimentel (errebepe) | Date: 2007-05-30 21:10 | |
Looks great! I don't mean to sound picky, but I think the comment should go inside the method definition. PEP8 says "Block comments generally apply to some (or all) code that follows them, and are indented to the same level as that code." But, really, now I'm just fine-tuning, the patch is fine as far as I'm concerned :) |
|||
msg52652 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2007-06-01 07:29 | |
Test methods shouldn't have docstrings. They screw up the output when running regrtest in verbose mode. Thanks for the patch! This patch failed when running under regrtest (./python -E -tt ./Lib/test/regrtest.py test_distuils) due to output from the distutils command. I just replaced stdout and everything worked. I modified it a bit (made test method name more descriptive and simplified logic a bit). Let me know if I screwed anything up. This still needs to be backported. I'll do it tomorrow unless someone else gets to it first. Committed revision 55731. (2.6) |
|||
msg52653 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2007-06-02 18:54 | |
Committed revision 55747. (2.5) |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:56:24 | admin | set | github: 44965 |
2007-05-17 18:13:58 | draghuram | create |