Skip to content

Commit

Permalink
Restrict visibility of all members of `CommandLines.ParamFileActionIn…
Browse files Browse the repository at this point in the history 
…put` to private.

Also clean up `CommandLinesTest` to use existing public getters for `CommandLines.ParamFileActionInput` and expand the checks on argument lists to validate the order.

PiperOrigin-RevId: 465655870
Change-Id: I76eb3fbd5d50152e3b16eeb791af5024aa2846e4
  • Loading branch information
@alexjski @copybara-github
alexjski authored and copybara-github committed Aug 5, 2022
1 parent 6b2915b commit fe7deab
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 20 deletions.
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,10 +276,10 @@ public List<ParamFileActionInput> getParamFiles() {

/** An in-memory param file virtual action input. */
public static final class ParamFileActionInput implements VirtualActionInput {
final PathFragment paramFileExecPath;
final Iterable<String> arguments;
final ParameterFileType type;
final Charset charset;
private final PathFragment paramFileExecPath;
private final Iterable<String> arguments;
private final ParameterFileType type;
private final Charset charset;

public ParamFileActionInput(
PathFragment paramFileExecPath,
Expand Down
38 changes: 22 additions & 16 deletions src/test/java/com/google/devtools/build/lib/actions/CommandLinesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ public void expand_simpleCommandLine_returnsCorrectCommandLine() throws Exceptio
ExpandedCommandLines expanded =
commandLines.expand(artifactExpander, execPath, NO_LIMIT, CommandAdjuster.NOOP, 0);

assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar");
assertThat(expanded.arguments()).containsExactly("--foo", "--bar");
assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar").inOrder();
assertThat(expanded.arguments()).containsExactly("--foo", "--bar").inOrder();
assertThat(expanded.getParamFiles()).isEmpty();
}

Expand All @@ -56,8 +56,8 @@ public void expand_commandLineFromArguments_returnsCorrectCommandLine() throws E
ExpandedCommandLines expanded =
commandLines.expand(artifactExpander, execPath, NO_LIMIT, CommandAdjuster.NOOP, 0);

assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar");
assertThat(expanded.arguments()).containsExactly("--foo", "--bar");
assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar").inOrder();
assertThat(expanded.arguments()).containsExactly("--foo", "--bar").inOrder();
assertThat(expanded.getParamFiles()).isEmpty();
}

Expand Down Expand Up @@ -88,10 +88,12 @@ public void expand_paramFileUseAlways_returnsCommandLineWithParamFile() throws E
ExpandedCommandLines expanded =
commandLines.expand(artifactExpander, execPath, NO_LIMIT, CommandAdjuster.NOOP, 0);

assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar");
assertThat(commandLines.allArguments()).containsExactly("--foo", "--bar").inOrder();
assertThat(expanded.arguments()).containsExactly("@output.txt-0.params");
assertThat(expanded.getParamFiles()).hasSize(1);
assertThat(expanded.getParamFiles().get(0).arguments).containsExactly("--foo", "--bar");
assertThat(expanded.getParamFiles().get(0).getArguments())
.containsExactly("--foo", "--bar")
.inOrder();
}

@Test
Expand All @@ -107,7 +109,7 @@ public void expand_paramFileCommandWithinLimits_returnsNoParamFile() throws Exce
ExpandedCommandLines expanded =
commandLines.expand(artifactExpander, execPath, NO_LIMIT, CommandAdjuster.NOOP, 0);

assertThat(expanded.arguments()).containsExactly("--foo", "--bar");
assertThat(expanded.arguments()).containsExactly("--foo", "--bar").inOrder();
assertThat(expanded.getParamFiles()).isEmpty();
}

Expand All @@ -127,7 +129,9 @@ public void expand_paramFileCommandOverLimits_returnsParamFile() throws Exceptio

assertThat(expanded.arguments()).containsExactly("@output.txt-0.params");
assertThat(expanded.getParamFiles()).hasSize(1);
assertThat(expanded.getParamFiles().get(0).arguments).containsExactly("--foo", "--bar");
assertThat(expanded.getParamFiles().get(0).getArguments())
.containsExactly("--foo", "--bar")
.inOrder();
}

@Test
Expand All @@ -151,11 +155,11 @@ public void expand_mixOfCommandLinesAndParamFiles_returnsCorrectCommandLines() t
assertThat(expanded.arguments())
.containsExactly("a", "b", "@output.txt-0.params", "e", "f", "@output.txt-1.params");
assertThat(expanded.getParamFiles()).hasSize(2);
assertThat(expanded.getParamFiles().get(0).arguments).containsExactly("c", "d");
assertThat(expanded.getParamFiles().get(0).paramFileExecPath.getPathString())
assertThat(expanded.getParamFiles().get(0).getArguments()).containsExactly("c", "d").inOrder();
assertThat(expanded.getParamFiles().get(0).getExecPathString())
.isEqualTo("output.txt-0.params");
assertThat(expanded.getParamFiles().get(1).arguments).containsExactly("g", "h");
assertThat(expanded.getParamFiles().get(1).paramFileExecPath.getPathString())
assertThat(expanded.getParamFiles().get(1).getArguments()).containsExactly("g", "h").inOrder();
assertThat(expanded.getParamFiles().get(1).getExecPathString())
.isEqualTo("output.txt-1.params");
}

Expand All @@ -176,10 +180,10 @@ public void expand_commandsWithParamFilesSecondExceedsLimits_returnsParamFileFor
commandLines.expand(
artifactExpander, execPath, new CommandLineLimits(4), CommandAdjuster.NOOP, 0);

assertThat(commandLines.allArguments()).containsExactly("a", "b", "c", "d");
assertThat(expanded.arguments()).containsExactly("a", "b", "@output.txt-0.params");
assertThat(commandLines.allArguments()).containsExactly("a", "b", "c", "d").inOrder();
assertThat(expanded.arguments()).containsExactly("a", "b", "@output.txt-0.params").inOrder();
assertThat(expanded.getParamFiles()).hasSize(1);
assertThat(expanded.getParamFiles().get(0).arguments).containsExactly("c", "d");
assertThat(expanded.getParamFiles().get(0).getArguments()).containsExactly("c", "d").inOrder();
}

/** Filtering of flag and positional arguments with flagsOnly. */
Expand All @@ -201,6 +205,8 @@ public void expand_flagsOnly_movesOnlyDashDashPrefixedFlagsToParamFile() throws
assertThat(commandLines.allArguments()).containsExactly("--a", "1", "--b=c", "-2");
assertThat(expanded.arguments()).containsExactly("1", "-2", "@output.txt-0.params");
assertThat(expanded.getParamFiles()).hasSize(1);
assertThat(expanded.getParamFiles().get(0).arguments).containsExactly("--a", "--b=c");
assertThat(expanded.getParamFiles().get(0).getArguments())
.containsExactly("--a", "--b=c")
.inOrder();
}
}

0 comments on commit fe7deab

Please sign in to comment.