Comment out native tools restore on Unix #2911
Conversation
|
NativeToolsRestore hangs on Unix that is a problem for CoreCLR. Is there any repo that is restoring native tools on Unix? Adding |
|
More context: dotnet/coreclr#24333 (comment) |
|
I think that disabling this is likely to break CoreFx. @ViktorHofer ? |
|
We don't use it yet but we planned to. |
|
@jkotas, is it not reasonable to define |
|
|
|
This sounds fine, the only issue is that it's a breaking change so we need to send notification before we merge this change. FYI @tmat |
|
I believe we should disable it on both Unix and Windows to be consistent. CoreFx isn't a consumer yet, I don't know if anybody uses that feature yet besides coreclr in their CI. |
|
I think wpf has a dependency on native-toolset installation and would have to make changes to adapt to this change of default. FYI @vatsan-madhavan I'm not certain who else is impacted. |
|
I want to help do the right thing here, but I'm not certain that I'm the right person to drive this. @MattGal , is this something FR should handle? I'm honestly not sure if this falls into FR duties or not. Also, I'm not saying that I'm going to drop this on the floor, I really do want to make sure we get to a reasonable solution. Answering this might help #2673 (comment) |
|
Please hold this PR for a few days - I don't have time to assess this until sometime next week. |
|
@jkotas, is it reasonable to make this change in your repo and not take Maestro updates for a few days (or manually merge them since you'll potentially have file conflicts)? |
|
We already do this for the time being, and there's no automerge on coreclr. If it breaks wpf, it might be easier for us to do the usual work around as long as this gets fixed soon. |
|
I just scanned the changes - looks like the change is to /cc @rladuca |
|
@hoyosjs , if native toolset bootstrapping is a problem, any reason not to just opt out of it entirely by removing any entries from your global.json? |
|
Also, can you point me to a specific build which is hitting a timeout / time increase so I can understand this better? Seems silly for cmake or python install to take 40+ minutes. |
We need it on Windows
Or in general arcade updates before we comment that part out. Last time I started trying to dig into the issue we get to this https://github.com/dotnet/coreclr/blob/34805dc3852ba218fcfd695bc0b9eb996860e025/build-packages.sh#L129 and then we start seeing the hangs. I didn't have a particularly good setup so I couldn't try to take a look at what was causing the hang then. |
|
@hoyosjs , are you ok with holding off on making any changes here until the Coreclr failure has been investigated and is better understood? |
I tend to look at these PRs, I can do the fix if I see them. @jkotas also gets to these before I do more often than not, and he originally modified the files himself. This is a breaking change, so I know it is not easy to take in. I'm okay with working around it for a bit if it's being actively investigated. |
FWIW, I looks a this as undoing a breaking change. The original change that introduced the problematic behavior was a breaking change. |
|
It was enabled long ago in the repo toolset I believe, but that was before coreclr being moved even a bit, think June timeframe from the history I see. Back then almost no one used this - in fact even now it's rarely used. But now that there's more consumers, I can see why we shouldn't right before a preview snap if the workaround is so straightforward. |
|
@jkotas, completely agree that the previous change was a breaking change (for the record that was undoing a breaking change, originally this was enabled by default but that got lost in a refactor; so it was undoing a breaking change that was undoing another breaking change). Sadly (or not), somewhere in the near past, we decided we should be more intentional and not callously make breaking changes. Thanks for understanding! |
|
Superseded by #2923 |
No description provided.