-
Notifications
You must be signed in to change notification settings - Fork 5.1k
System.Formats.Cbor.Tests: enable the project to find the SDK-bundled FSharp.Core package. #115471
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
… FSharp.Core package.
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 have no idea what this is doing. @dotnet/fsharp-team-msft can you please take a look?
My main feedback here would be why encode all this specialness into this one fsproj when there could be others in the repo or stack?
This does not make much sense to me either, why cannot it pick the same FSharp.Core.dll which came with the dependency (when it is not an F# compiler where compiler itself would ensure it). |
When an However, this is a This PR enables the
It's special because it's not an And because this is needed only when the And because this
This is the problem I'm trying to address (from #115469):
With this change, the Does this option make sense now? Or what is a better option? |
Does this imply that there's a strong dependency between what fsharp inserts into the .NET SDK and what is available on the package feeds? That sounds like a contract violation to me as fsharp ships independently. cc @mmitche |
Do you depend on a public FsCheck version? |
I don't think that's an issue. F# projects are happy to use what comes with the SDK.
runtime repo uses the 2.14.3 version from nuget.org.
What I guess might be the happening is that restore prefers to use the highest version available for the package for all feeds, but then runs into an issue when the project being restored can't actually access the feed that provides that version. So ... this may be a restore issue that happens under special circumstances: same package available from different feeds in different versions while not all projects can access those feeds. I doubt that if there is a more general restore issue, that it would get solved on account of this issue alone. It would need more of an end-user impact. I'd like to handle this specific case as the PR is doing. In a released .NET version, when |
I hope from the discussion it is clear that this is addressing a very specific case, and that this change is directly meeting the expectation of the SDK by making it find the nuget package it expects. I don't want to spend time on the SDKs behavior here because I doubt it's going to change just for this specific case. |
I would expect the released version of any .fsproj in the SDK to use the source-built F# compiler in the VMR, and use FSharp.Core from that fresh F# compiler build- which might be before it is released to nuget for public consumption (@ViktorHofer: Am I seeing this correct?). |
That is so. The special handling in this PR is because this is a C# |
What I struggle to understand is why this change is required now and wasn't required before. Any idea? I initially thought this was related to VMR packages publishing to the wrong feeds. Still not sure whether it is or not. |
Often this is unnecessary because the FSharp.Core version is available on Microsoft CI feeds. It's needed only when someone source-builds an SDK, that SDK has an FSharp.Core version that is not on Microsoft CI feeds, and then that SDK gets used to build runtime repo. Our internal runtime CI builds for s390x/ppc64le do this, and they are currently running into this problem. When the build script gets you an SDK (for the Microsoft supported architectures), there's always a matching FSharp.Core version available on the CI feeds. |
The SDK's FSharp dependency doesn't ship independently. FSharp provides packages it wants to ship to the SDK, and the SDK publishes them as its "own". |
@ViktorHofer if you take an x64 SDK built from vmr main and try to use it to build this repo, you'll run into #115469. We don't run into the issue for x64 because we just use Microsoft's SDK for that. For community architectures, we do use our own SDK and to have one that is sufficiently new for runtime, we use the latest one we've built from vmr main. |
@tmds thanks for bearing with us and responding to all our questions so far. We are trying to understand this case better :)
I'm unsure how that could be the case given that we have https://github.com/dotnet/sdk/blob/bf4223b2e37bce209c490c63f677d20fdf5a090e/eng/Publishing.props#L27-L28 which publishes the packages to the dotnet10 feeds as part of sdk's publish. |
From our CI builds (and the error message) vmr main is building FSharp.Core Do you find that version on the dotnet10 feeds? Based on the error message, I assume it's not there. |
It's not... I think I kind of see what is going on:
This might be a hole in source build? I would expect SB's build of main VMR to be dependent on the FSharp version that is produced in SB, not based on one encoded into the VMR. Maybe this is to be fixed with @mthalman's ongoing work. Either way...@tmds once we fully get SB off the "fixed" versions that used to be specified by the Microsoft build, a random, non-stable SB build of .NET won't work without also providing the packages produced by that SB build too. Think of the MS build and SB build as two separate entities. If they agree on Official Build ID or are stable, meaning the OBID does not affect the identity of the packages that the product depends on, then MS's packages could be used with the SB-built SDK. |
Understood, when this change happens, I should expect other packages to no longer be available on nuget feeds either. I'll have to bring those along for building runtime. @mmitche @ViktorHofer are you ok with taking this PR as a stopgap? |
As a band aid, sure. But we should probably revert this later when the versioning strategy is figured out for SB. We should probably add a TODO with a tracking issue. |
Thanks! This will unblock our internal CI. I'll revert this when I update our CI to handle the versioning changes. |
I chatted with @mmitche about this offline. I just want to underscore here as well that we must never publish SB packages anywhere - they must never escape the build. That's the reason for the poison infrastructure in the VMR. SB produced packages are completely different than MSFT produced packages (thinking about TFMs). This probably means that for SB CI / DEV or one-off builds with a different input version, we will always also need to supply a MSFT baseline version to restore packages from. |
That makes sense. As long as there is a way for us to do CI, it's good. We'll deal with the changes as they are applied upstream. |
I've changed this to draft. I'm trying a quick fix in our internal CI scripts. If it works I'll close this. |
The quick fix is to copy the This enables the |
Sounds good, thanks for following-up. Sounds like the fsharp scenario is an online one and always require package downloads? If there is desire to make this an offline one, we could bundle the package similar to illink.tasks and crossgen2. |
It is bundled, but in a way that only This quick fix is to copy it to the location where illink.tasks is so it gets picked up for the |
I see, that's the nested library-packs folder, got it. Does anything speak against hoisting that inner out into the outer library-packs folder? |
It makes sense to look into that. It enables using the package in more case. I can't think up reasons not to do it. |
Fixes #115469.
@ViktorHofer ptal.