-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add subdir #11
Add subdir #11
Conversation
Copies files to the subdirectories of the build target dir. Xuefeng and I worked on this and tested it to make sure it works.
Updated to add comments from the original config_build.txt
My notes while reviewing Line numbers below refer to the Header block is missing labels. See the code style guidelines. The pull request summary mentions the '-a' option. I don't see this option in From which directory is this script executed? (we should at least mention this Line 305 states that the current directory (i.e., ".") contains the sources of the targets Future reference: Try to avoid os.chdir() as it leads to readability issues The script also does things like create subdirectories (repyV1 and repyV2) line 21: build.py? Should this be build_component.py? line 232: mentions preparetest.py (module name as been changed to line 234: This comment states an option should be added, but it's not implemented? line 260: os.path.realpath() is used in the code, so "canonical path" (rather line 308: Remove this commented line of code. I came across a few others in this module. line 384: Unclear why we are changing to this directory? Was the module originally |
Thanks for your feedback. Here's a bunch of comments.
Valid, but not subject of this PR. Let's keep things cleanly separated.
That's an artifact of this PR also including functionality of #7 to copy over directory trees. I've discussed this with @priyam3nidhi already, and she will amend the PR accordingly.
Good point, although the expected format is
Indeed, it must not do this anymore, unless (obviously) requested by the build config to do so. Also, invocations of
See above, the former is a wrapper script for the latter.
ACK!
See above, #7 / #8 confusion / mixup.
Indeed, but not subject of this PR.
Agreed, this shouldn't have been there in the first place.
I believe this is an issue of the code being updated, but not the comments. I'd need to look at an earlier revision to know what we were doing then. |
Added the functionality that allows to copy files to subdirectories of the build target dir.
Issue: #8
To verify: Create an empty directory with a few files. Create another directory. Use -a to copy from dir1 to dir2. Cross check to see that dir2 now has the contents of dir1.
To test: Add -a for build instructions as mentioned in the docstring