Skip to content
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

Merged
merged 5 commits into from
Nov 7, 2014
Merged

Conversation

priyam3nidhi
Copy link
Contributor

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

Priyam and others added 5 commits September 29, 2014 22:47
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
@vladimir-v-diaz vladimir-v-diaz merged commit 6775b4a into SeattleTestbed:master Nov 7, 2014
@vladimir-v-diaz
Copy link
Contributor

My notes while reviewing build_component.py and pull request #11.

Line numbers below refer to the build_component.py available here:
https://github.com/SeattleTestbed/common/blob/6775b4a024b40a356a53042a99e2a3e16be86ba0/build_component.py

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
the header block. Was it previously an option that was later discarded? Is
the sub-directory feature now expected to live in "config_build.txt"? Please update
the pull request's summary and/or add the latest information to the header block.
Also mention the expected format of this feature:
"test ../relative/path/".

From which directory is this script executed? (we should at least mention this
in the header block). path/to/component/DEPENDENCIES/? NOTE: I
found the format of directory paths specified in the comments unclear. Are instances
of ../../ needed?

Line 305 states that the current directory (i.e., ".") contains the sources of the targets
built. I had to backtrack and verify what the current directory is because of the os.chdir() calls. Maybe explicitly specify the paths (e.g., RUNNABLE directory?)

Future reference: Try to avoid os.chdir() as it leads to readability issues
imo. I think this function could have been avoided in many places.

The script also does things like create subdirectories (repyV1 and repyV2)
in the targets directory. Should mention this in the header block. I noticed these directories existed in other built components and wondered which script created them: now I know but I had to go deep into the code to find it.

line 21: build.py? Should this be build_component.py?

line 232: mentions preparetest.py (module name as been changed to
'build_component.py')

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
than absolute) is more appropriate.

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
in this directory? According to the comments, 'repos_root_dir' is
"path/to/component/DEPENDENCIES/".

@aaaaalbert
Copy link
Contributor

Thanks for your feedback. Here's a bunch of comments.

Header block is missing labels. See the code style guidelines.

Valid, but not subject of this PR. Let's keep things cleanly separated.

The pull request summary mentions the '-a' option. (...)

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.

From which directory is this script executed?

build_component.py is not executed directly, but called via build.py, which takes a few steps to adjust paths to match the expected layout.

Also mention the expected format of this feature: "test ../relative/path/".

Good point, although the expected format is

  • SOURCE_DIR (with the target dir implicit from command-line options, or the default ../RUNNABLE)
  • test SOURCE_DIR (again with implicit/default target, but for unit tests)
  • SOURCE_DIR SUBDIR_OF_TARGET_DIR (the feature that this PR should add)
  • test SOURCE_DIR SUBDIR_OF_TARGET_DIR (the feature that this PR should add)
    This needs to also go into the BuildInstructions wiki.

The script also does things like create subdirectories (repyV1 and repyV2)

Indeed, it must not do this anymore, unless (obviously) requested by the build config to do so. Also, invocations of repypp.py should be removed.

line 21: build.py? Should this be build_component.py?

See above, the former is a wrapper script for the latter.

line 232: mentions preparetest.py (module name as been changed to 'build_component.py')

ACK!

line 234: This comment states an option should be added, but it's not implemented?

See above, #7 / #8 confusion / mixup.

line 260: os.path.realpath() is used in the code, so "canonical path" (rather than absolute) is more appropriate.

Indeed, but not subject of this PR.

line 308: Remove this commented line of code. I came across a few others in this module.

Agreed, this shouldn't have been there in the first place.

line 384: Unclear why we are changing to this directory? Was the module originally in this directory? According to the comments, 'repos_root_dir' is "path/to/component/DEPENDENCIES/".

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants