-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix no-argument ./build.sh invocation #116315
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
Conversation
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.
Pull Request Overview
This PR ensures that running ./build.sh
without any arguments no longer fails by initializing the arguments
array with an empty element.
- Changed default
arguments
initialization to include an empty string. - Preserves existing behavior of passing flags via the
arguments
array.
Comments suppressed due to low confidence (1)
eng/build.sh:156
- Using an empty string in the arguments array can lead to a spurious empty-argument being passed downstream. Consider explicitly detecting when no arguments are provided and handling that case, rather than injecting an empty element into the array.
arguments=("")
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
/ba-g fast merge as this is apparently not covered by CI otherwise we'd have seen it in #116145 and I verified it works locally. |
@omajid do we need to make similar changes for the other cases like this you added in dotnet/dotnet#677 and dotnet/dotnet#797 ? |
@jkoritzinsky I think this fix is not quite right. This adds a single empty argument. That might break something. @akoeplinger I thought I had fixed it via https://github.com/dotnet/dotnet/pull/797/files#diff-56c552fdb744de6c93ad3afef9db9a34839f7b4b0e0d3f653b41310dd62776a3. Is there something I need to do to help the fix flow in here? |
Ah, I see. I think we need to guard this as well: Line 590 in f03b2ab
|
The for loop on L590 seems to work fine as is from what I can see |
Follow-up to dotnet#116315 (comment)
Thanks for finding and fixing it! |
Follow-up to #116315 (comment)
Fixes issue reported in #116145 (comment)