Added checks for main dependencies on runtime's build scripts #39052
Conversation
eng/build.ps1
Outdated
| function Assert-InstalledDependency($dependencyName) | ||
| { | ||
| Write-Host "Checking for $dependencyName..." | ||
| if (((Get-ItemProperty HKLM:\Software\Microsoft\Windows\CurrentVersion\Uninstall\*).DisplayName -Match |
There was a problem hiding this comment.
I have all these dependencies (except Visual Studio) installed via xcopy on my machine. I do not have any of this in registry. I know number of other folks on the team are using similar setup. CI is using this setup too.
This should check whether the dependency is on the path.
eng/build.sh
Outdated
| assertInstalledDependency 'python3' | ||
| assertInstalledDependency 'git' | ||
| assertInstalledDependency 'cmake' | ||
| echo "All dependencies fulfilled! Initiating build..." |
There was a problem hiding this comment.
It is really necessary to prints this every time?
|
Tagging subscribers to this area: @ViktorHofer |
|
Cmake and Python we already check for them. The build fails but continues to run, that's why the build throws cryptic errors. That will be fixed by: microsoft/MSBuildSdks#186 CMake doesn't just need to be installed, it requires a minimum version on Windows and another Version on Unix. I think we should just leverage the CMake error to the pieces of the repo that actually use CMake and the Python one to the pieces that just use Python. For example, if I just want to build for WASM, I don't need to have installed Python. |
|
Would it be substantial work to eliminate the python dependency? |
What would you replace it with? We use it for generating of eventing code at build time and for coreclr test execution. These need to work even on platforms where .NET doesn't exist yet and is being brought up. So using powershell or msbuild is not possible there. While we could possibly use sh / cmd scripts, it would mean duplication of the code. So python sounds like the best thing to use due to its widespread availability. I think it might be possible to convert the eventing code generation to cmake script, but we'd still need python for running coreclr tests. |
For bootstrapping build, we can have a file with empty event firing stubs checked in (or we can be a very trivial script written to generate this). And once we are fully bootstrapped, the full eventing code generator can be written in C#.
The regular coreclr test execution should not need python. It is using xunit as the driver. |
|
Thanks for the feedback everyone. I have some doubts regarding the current state of things. @safern mentioned Python and CMake will be checked in their appropriate places with the PR he's working on. Also, taking into account @jkotas observation on Python being only needed for certain scenarios, I assume that means I should remove them from this one? This leads me to another question. That would leave to only check for Git, Long Paths, and Visual Studio? And look at the PATH instead of the registry? |
|
FYI: #39481 is fixing the bug that we have where we don't stop on the first subset failure in the traversal with multiple subsets. So after that change the checks that coreclr has in |
|
@ivdiazsa do you have the answers you need to complete this PR? It seems, remove Python and check only using PATH. As an aside do we depend on VS really, or is it just a proxy for acquiring various SDK's? |
Discussion stopped and I got busy with other work items. My bad for not following up more closely on this one. The thing is as far as I'm concerned, when one builds the runtime repo, it calls on VS Developer Command Prompt. If this is true, then we do need VS for Windows building. Can you clarify this @jkotas ? Regarding the other dependencies, so we just remove Python and check PATH right? @danmosemsft |
|
If the build today will fail without python then we should check for it. My impression was that the discussion was about removing that dependency later. I didn't realize we depend on the developer prompt : in the libraries build we don't (cc @ViktorHofer) |
|
Right! From my understanding, the Python dependency was going to be taken care off in another PR somewhere else. We just need now clarification for the Dev Command Prompt. I'm almost sure we do but I'm not certain under which scenarios or if I'm just wrong on this. |
|
@jkoritzinsky can confirm whether VS prompt is required by any part of the build. |
|
All native build components require VS prompt. Libraries native build calls it here: https://github.com/dotnet/runtime/blob/master/src/libraries/Native/build-native.cmd#L60 Each native build component has the VS prompt setup duplicated. Discussion about unifying is happening here: #38919 (comment) |
|
I have now changed the dependency checks on Windows to go to the PATH, instead of the registry. Also, removed Python as that was going to be taken care of in another PR. While working on this, I got a couple more questions:
|
|
cc @trylek for the setup vs tools script question (and potential convergence). |
On Unix (in the .sh scripts), the presence is checked via command -v ninja 2>/dev/null || command -v ninja-build 2>/dev/null || { echo "Unable to locate ninja!"; exit 1; }
The PATH on my Ubuntu 16.04 contains these paths, the cmake, python and others are in |
|
In a future change, it would be good to also check that the correct VS workloads are present. To help with eg #39787 (comment) |
… added a check script for VS.
eng/vs_tools_check.cmd
Outdated
| @@ -0,0 +1,40 @@ | |||
| @if not defined _echo @echo off | |||
There was a problem hiding this comment.
Would it be better to add an argument to the existing string instead of duplicating the whole existing script except the one line that calls VsDevCmd.bat ?
There was a problem hiding this comment.
Sorry I hadn't seen this before Jan. Yes, I think this is a good idea to try. I also wasn't very comfortable with duplicating the code but wasn't able to think of another approach. Will try this.
|
|
||
| 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.
The message refers to linux-requirements.md, but the current OS can be OSX.
There was a problem hiding this comment.
Nice catch. I will add a check and point to OSX when it's such case.
| :: VS installation, and not launch the Dev Prompt. More details are in that | ||
| :: script source file. | ||
|
|
||
| call "%~dp0src\coreclr\setup_vs_tools.cmd" 1 |
There was a problem hiding this comment.
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.
Also, isn't there an Arcade script that already does that?
There was a problem hiding this comment.
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.
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.
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.
| } | ||
|
|
||
| assertInstalledDependency 'CMake' | ||
| assertInstalledDependency 'Git' |
There was a problem hiding this comment.
Git isn't a dependency. Sourcebuild needs to work without git.
There was a problem hiding this comment.
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.
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.
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.
So we just check for CMake then?
|
@ViktorHofer Are the unconditional dependency checks for native tools going to be a problem for your effort to reduce bar for non-inbox library contributions? These native tools should not be required for non-inbox library contributions. |
Agreed. I'm not a big fan of front-loading these checks from the perspective of libraries as in 99% of the time we don't need cmake. In more detail, with this change we won't be able to build subsets that don't depend on these toolsets. |
|
Yeah I was going to suggest the same thing, maybe we should just check for cmake, if subset contains |
|
The subset system is something sitting on top of our repository for the sake of building individual parts easier. I don't think we should use it to determine when to check for native toolsets being installed. What's so bad about checking just-in-time where they are used? We have a couple of dependencies on Linux that we probably don't want to check for individually. I'm curious why we are adding checks for some like cmake? |
Yeah, I agree that would be better... that is what we do for python checks on coreclr, and now after I fixed the issue wrt StopOnFirstFailure, that shouldn't be a problem, because as soon as a dependency is checked when we need it, it will fail at that point in time with a clear error message without continuing the build. |
|
I was reading the latest comments on this thread and I'm a bit confused regarding the way to proceed. This is what I understood from the discussion:
|
Which comment above suggests that we should check for CMake in the sh version only? I would expect that we want to check for cmake on both Windows and Unix; or on neither of them. |
|
My opinion is same as @ViktorHofer. I believe that these upfront checks do more harm than good, and it is better to just fail at the point where the tool is used. Yes, it means that you may not get the error message about missing tool right away. If this is the general agreement, then this PR and issue can be closed. |
|
The original issue got several thumbs up. It is a poor experience to build to an error, satisfy a dependency, rebuild for a few minutes, hit another and repeat. We try to document these but it is easy to not have things set up right. Also the error messages are not necessarily clear. I get that some dependencies are not relevant for libraries. Can we not identify certain dependencies that are likely required if you do native code work, and check for those together if you do such a build? We do not add and remove such dependencies much. In my mind it doesn't have to be perfect to be better . |
|
As an experiment, I have renamed cmake.exe to see the error I get from build.cmd with wrong or missing cmake. Unfortunately, it is still: The crossgen error is in red. The message about missing cmake is two screens above this error in while and very hard to find. The PR in the current shape is not going to make this better. I got this error message after 2.5 minutes. Most of this time is spent in restoring packages. This time can be a lot more without fast internet - downloads close to 1GB. |
|
I think we should be fixing the error reporting we have instead of introducing a redundant one. The checks left in this PR are about validating that you got good cmake and C++ compiler. The best way to validate that you got a good cmake and C++ compiler is to compile something and run it. My proposal is that we create a new subset that is built before the package restore step. This subset should compile a "hello world" using cmake, run it and fail the build if anything goes wrong. This design should have mimimal duplication and it will ensure that the tools actually work (e.g. that cmake is of the right version, etc.) |
... Sounds like that still leaves the error two screens up? What @jkotas suggests seems reasonable if it ends up with something clearer then "in white two screens up" BTW Where did we end with python? Do we need an issue to remove it? Will my build succeed without it today if my build does not need it? |
Right that needs to be fixed too. |
|
| @@ -116,6 +128,8 @@ if ($subset -eq 'help') { | |||
| exit 0 | |||
| } | |||
|
|
|||
| Assert-InstalledDependency("Git") | |||
There was a problem hiding this comment.
Git isn't a dependency. The repo should be buildable without any source control provider. #47298 tracks protecting this scenario in CI.
|
Based on what we discussed earlier, the only native dependency that we would check for is CMake. Also we only want to check for it, if the projects to be built, requires it (i.e. managed libraries don't). I'm not sure if the work required to do so is worth the implementation cost. This PR, in it's current state isn't ready to be merged. That said, thanks @ivdiazsa for your work, which triggered quite interesting discussions. |
|
Let's close this then |
ghost
locked as resolved and limited conversation to collaborators
Fixes #38169.
This PR adds checks to ensure that Python 3, Git, CMake, and specifically for Windows, Visual Studio and Git Long Paths are enabled before attempting to perform any build of the repo.
With these safety measures, developers/users will no longer face the problem of trying to build, and be greeted much time later with strange error messages because a certain dependency was not found.