-
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
allows nesting in the package name and print output from execute command #15
base: master
Are you sure you want to change the base?
Conversation
@AlexanderFabisch please check the pull request |
Maybe, we should only print the output in case of an error? |
@@ -83,7 +83,8 @@ def checkBaseName(package, info): | |||
if "$" in info["gitPackage"] and "*" not in package: | |||
info["basename"] = info["gitPackage"] | |||
info["gitPackage"] = info["gitPackage"].replace("$PACKAGE_BASENAME", | |||
package.split("/")[-1]) | |||
'-'.join((package.split("/")[1:]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are too many brackets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexanderFabisch do you mean readability of the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(package.split("/")[1:])
is not necessary.
@@ -117,7 +118,9 @@ def clonePackage(cfg, package, server, gitPackage, branch): | |||
cmd = ["git", "clone", "-o", "autobuild", "-q", server+gitPackage, clonePath] | |||
if branch: | |||
cmd += ["-b", branch] | |||
execute.do(cmd, cfg) | |||
out, err, r = execute.do(cmd, cfg) | |||
print out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexanderFabisch it is usefull to have output message for debug purposes. is there any debug mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. @malter should know this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexanderFabisch
what is the reason not to have output message for debug purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, this would be the only command that actually prints output to stdout. If we added a debug mode, that would be OK but that should be done in another pull request.
execute.do(cmd, cfg) | ||
out, err, r = execute.do(cmd, cfg) | ||
print out | ||
print err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @malter suggested:
if err:
print(err)
there is a bug with deep nesting package name, e.g. gui/osgviz/osgviz. since it took just the last split, the repository path was wrong. /gui-osgviz, but right is /gui-osgviz-osgviz
so this patch allows deeper nesting package name.