Skip to content
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

declare and bump DependencyModel to 8.x #13473

Merged
merged 3 commits into from
May 10, 2023

Conversation

oleksandr-didyk
Copy link
Contributor

Resolves #13471

Tested by building an intermediate of Arcade locally and giving it to runtime. Both builds succeeded without any issues.
Will create a subscription for the live version after the PR is approved.

@MichaelSimons MichaelSimons requested a review from mthalman May 9, 2023 21:52
mthalman
mthalman previously approved these changes May 10, 2023
MichaelSimons
MichaelSimons previously approved these changes May 10, 2023
@@ -42,7 +42,7 @@
<!-- runtime -->
<MicrosoftExtensionsDependencyInjectionAbstractionsVersion>6.0.0</MicrosoftExtensionsDependencyInjectionAbstractionsVersion>
<MicrosoftExtensionsDependencyInjectionVersion>6.0.0</MicrosoftExtensionsDependencyInjectionVersion>
<MicrosoftExtensionsDependencyModelVersion>6.0.0</MicrosoftExtensionsDependencyModelVersion>
<MicrosoftExtensionsDependencyModelVersion>8.0.0-preview.5.23259.1</MicrosoftExtensionsDependencyModelVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you choose this version? Why is the previous 6.0.0 version not simply overridden by the previously source built version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous source-build artifacts don't apply to repo source-build legs. e.g. this was not a problem in the product source-build rather it is a problem when consuming the arcade intermediate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh. It's a bit annoying that we have to ensure we choose a version that is not in SBRP. I wonder if there is a better way to do this...would be fantastic to have a way of annotating which inputs should come from SBRP or otherwise.

@ViktorHofer
Copy link
Member

ViktorHofer commented May 24, 2023

IMHO this change looks wrong. Arcade shouldn't depend on an 8.0 Preview package. 7.0.0 should be sufficient. Can you please explain why this change was necessary?

@oleksandr-didyk
Copy link
Contributor Author

oleksandr-didyk commented May 24, 2023

Can you please explain why this change was necessary?

The linked issue has an adequate outline for why this was done originally - #13471

@ViktorHofer
Copy link
Member

I did read the linked issue but I still don't understand why we change the version in a non source build environment.

Since we cannot use a historic version and we cannot get rid of the dependency (see dotnet/runtime#84925 (comment)), we need to utilize the latest version of the package. In this case, an intermediate would be used for the dependency, allowing it to be loaded during the build.

Why don't we use the N-1 toolset version as we do with other dependencies? Why do we force the non source build to use a preview version of a package? That goes against what we usually do in Arcade (depend on the latest stable version).

@oleksandr-didyk
Copy link
Contributor Author

oleksandr-didyk commented May 25, 2023

Why don't we use the N-1 toolset version as we do with other dependencies?

We are. Version N-1 is the version of the latest available source build of a given component. In the context of repo build, this would mean the latest version available at the time of the build, which can be a preview version. N-1 here does not refer to the previous release though.

Why do we force the non source build to use a preview version of a package?

Ideally we want the source-build and the non source-build to be identical. This allows us to surface issues as soon as possible so their impact is minimal + we are confident that there is no difference in behaviour between the two. Pre-built detection and repo PvP initiatives are both to some extent aimed at achieving this goal.

@mmitche
Copy link
Member

mmitche commented May 25, 2023

@oleksandr-didyk I think we should be choosing a version that aligns with the latest preview release, not the latest version. I don't think it's great to have a subscription from runtime->arcade.

In this case it would be the preview 4 release of Microsoft.Extensions.DependencyModel, 8.0.0-preview.4.23259.5

@oleksandr-didyk
Copy link
Contributor Author

oleksandr-didyk commented May 29, 2023

In this case it would be the preview 4 release of Microsoft.Extensions.DependencyModel, 8.0.0-preview.4.23259.5

Sure thing, can do - #13718

Is there some existing process that can guarantee that the repo maintainers would update this version after a new preview is available?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump DependencyModel to latest version
5 participants