-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Added checks for main dependencies on runtime's build scripts #39052
Changes from 5 commits
724cb29
2cc29b2
a2fc0bc
4f9a919
2ae488e
4c622d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,18 @@ function Get-Help() { | |
Write-Host "For more information, check out https://github.com/dotnet/runtime/blob/master/docs/workflow/README.md" | ||
} | ||
|
||
function Assert-InstalledDependency($dependencyName) | ||
{ | ||
try { | ||
Get-Command -Name $dependencyName -ErrorAction Stop >$null 2>&1 | ||
} | ||
catch { | ||
Write-Host "$dependencyName is required to build this repo. Make sure to install it and try again." | ||
Write-Host "For a full list of requirements, see https://github.com/dotnet/runtime/blob/master/docs/workflow/requirements/windows-requirements.md" | ||
exit 1 | ||
} | ||
} | ||
|
||
if ($help -or (($null -ne $properties) -and ($properties.Contains('/help') -or $properties.Contains('/?')))) { | ||
Get-Help | ||
exit 0 | ||
|
@@ -110,6 +122,8 @@ if ($subset -eq 'help') { | |
exit 0 | ||
} | ||
|
||
Assert-InstalledDependency("Git") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Git isn't a dependency. The repo should be buildable without any source control provider. #47298 tracks protecting this scenario in CI. |
||
|
||
if ($vs) { | ||
. $PSScriptRoot\common\tools.ps1 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,6 +136,21 @@ showSubsetHelp() | |
"$scriptroot/common/build.sh" "-restore" "-build" "/p:Subset=help" "/clp:nosummary" | ||
} | ||
|
||
assertInstalledDependency() | ||
{ | ||
dependency="$(echo "$1" | awk '{print tolower($0)}')" | ||
location="$(command -v $dependency)" | ||
|
||
if [ -z "$location" ]; then | ||
echo "$dependency is required to build this repo. Make sure to install it and try again." | ||
echo "For a full list of requirements, see https://github.com/dotnet/runtime/blob/master/docs/workflow/requirements/linux-requirements.md" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The message refers to linux-requirements.md, but the current OS can be OSX. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch. I will add a check and point to OSX when it's such case. |
||
exit 1 | ||
fi | ||
} | ||
|
||
assertInstalledDependency 'CMake' | ||
assertInstalledDependency 'Git' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Git isn't a dependency. Sourcebuild needs to work without git. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it is. The build uses it to extract it the commit hash that we embed into the binaries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it done unconditionally, or can this behavior be suppressed by configuration? I believe that there are configuration options to suppress this behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, I've tried to uninstall git and the build completed ok, just no version info got embedded into the binaries. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we just check for CMake then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
|
||
arguments='' | ||
cmakeargs='' | ||
extraargs='' | ||
|
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.
That doesn't seem right. We don't want to call into a coreclr script from the repo root. Can you extract that and move it into eng/?
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.
Also, isn't there an Arcade script that already does that?
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 did this in order to avoid code duplication, but now that you bring it up, it is probably better practice to keep root script functionality outside of any subsets of the repo. I can extract the checking functionality into a new script in
eng
. As far as I'm concerned, we do need to add this dependency check.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.
yes please do so.
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.
and please check if Arcade already offers something useful to check dependencies. And if not, we might want to add it to Arcade.
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.
We have #1251 opened on unifying this.
Arcade has https://github.com/dotnet/runtime/blob/master/eng/common/tools.ps1 that you probably had in mind. It is pretty far from what we need here.