Conversation
|
Add @mmitche as a reviewer. |
MichaelSimons
left a comment
MichaelSimons
left a comment
There was a problem hiding this comment.
What validation has been performed on these changes?
eng/common/build.sh
Outdated
| bl="/bl:\"$log_dir/Build.binlog\"" | ||
| fi | ||
|
|
||
| [[ $properties != *"/p:Configuration"* ]] && properties="$properties /p:Configuration=$configuration" |
There was a problem hiding this comment.
Aren't these case sensitive matches? Do we need to worry about doing a case insensitive match?
There was a problem hiding this comment.
This is a good point, please check the commit: 80a0a3e
|
I'm not sure I like this direction. We were discussing removing the inner-build eventually which would fix this issue: dotnet/source-build#4249 |
eng/common/build.sh
Outdated
| fi | ||
|
|
||
| [[ $properties != *"/p:Configuration"* ]] && properties="$properties /p:Configuration=$configuration" | ||
| [[ $properties != *"/p:RepoRoot"* ]] && properties="$properties /p:RepoRoot=\"$repo_root\"" |
There was a problem hiding this comment.
Can this logic be put into a function that get's called? Something like:
function AddPropertyIfMissing {
local property_name=$1
local property_value=$2
if [[ $properties != *"/p:$property_name"* ]]; then
properties="$properties /p:$property_name=$property_value"
fi
}
...
AddPropertyIfMissing "Configuration" "$configuration"
| <Target Name="RunInnerSourceBuildCommand" | ||
| DependsOnTargets="PrepareInnerSourceBuildRepoRoot"> | ||
| <ItemGroup Condition="'$(BaseInnerSourceBuildCommand)' == ''"> | ||
| <InnerCommands Include="$(ARCADE_BUILD_TOOL_COMMAND.Split(' '))" /> |
There was a problem hiding this comment.
I don't think this will work...if there is a space in a command (e.g. a file path), it'll split on that.
mmitche
left a comment
mmitche
left a comment
There was a problem hiding this comment.
I don't think this needs to be all that complex. I think that the main duplication we see is from this block:
Where we re-pass args that are already present. e.g. ContinuousIntegrationBuild
I would test this in the VMR build first. The changes are trivial to make in the arcade directory and then run a draft PR in the dotnet/dotnet repo.
|
Just like @ViktorHofer said, the dotnet/source-build#3789 will be fixed in the future: dotnet/source-build#4249. Close this pr. |
5 participants
Fix dotnet/source-build#3789
Remove the duplicated args from the outer repo build command if the args have been added to the inner repo build command.