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

Adds a run system command in a path macro and fixes existing docu #6250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

juergen-albert
Copy link
Contributor

Signed-off-by: Juergen Albert [email protected]

Copy link
Member

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this as a good idea. Sure it is no worse security wise than system, but will lead to non-portable builds. As soon as you start cd'ing to other random folders it assume things outside the Bnd workspace and that is probably non-portable and will lead to builds which work for those who have the proper folders in place but not everyone else.

}

# Extracts the current git SHA for the Project and enters the password as input
Git-SHA: ${system;git rev-list -1 --no-abbrev-commit HEAD;mypassword}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a helpful example since the command does not care about stdin :-)


public String system(boolean allowFail, String runDirectory, String command, String input)
throws IOException, InterruptedException, InterruptedException {
return system(allowFail, new File(runDirectory), command, input);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be IO.getFile(runDirectory) since that will handle some interesting cases like ~ expansion?

Comment on lines +1392 to +1394
@DisabledOnOs({
OS.MAC, OS.LINUX
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@DisabledOnOs({
OS.MAC, OS.LINUX
})
@EnabledOnOs(OS.Windows)


if (args.length > 2) {
input = args[2];
if (extractPath) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section looks overly complex and ripe for errors.

public void testSystemInPath() throws Exception {
try (Processor p = new Processor()) {
Macro macro = new Macro(p);
assertEquals("/tmp", macro.process("${system-in-path;/tmp;pwd}"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a hard coded path, you should create a folder in the @InjectTemporaryDirectory temp folder. Then these test can run on all OS.

---
layout: default
class: Macro
title: system-in-path ';' PATH ';' CMD ( ';' INPUT )?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro names are not awesome. Naming is hard :-)

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.

2 participants