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

#1250 cleaning of package reference's #1253

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thompson-tomo
Copy link
Contributor

@thompson-tomo thompson-tomo commented Jan 21, 2024

Description

The PR goes about cleaning up the package dependencies such that any project that uses a framework does not have an explicit reference to any library which is being supplied by the framework. At the same time, if a project has a dependency on a project which has a framework reference, that project also gains the framework. This approach improves manageability of the dependencies.

The abstraction packages have been left without a framework refernce.

Fixes #1250

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

@thompson-tomo thompson-tomo changed the title Enhancement/#1250 cleaning of packages /#1250 cleaning of package reference's Jan 21, 2024
@thompson-tomo thompson-tomo changed the title /#1250 cleaning of package reference's #1250 cleaning of package reference's Jan 21, 2024
@TimHess
Copy link
Member

TimHess commented Jan 24, 2024

Hi @thompson-tomo,

Some of these look good (primarily removing packages that aren't needed) but there are concerns with these changes:

  1. Is there a size impact when PublishSingleFile is used?
  2. We do not want to force a dependency on ASP.NET Core if it isn't needed (eg: console/worker apps don't need it)
    1. Using framework refs in the tests could create situations where the tests include more dependencies than a downstream app might and create an inaccurate test environment

I don't immediately have answers to those and haven't had time yet to develop an understanding of those impacts.

@thompson-tomo
Copy link
Contributor Author

thompson-tomo commented Jan 24, 2024

Hi @TimHess

The only projects which gained a framework reference are those which already had a project dependency on another steeltoe project which had a framework reference hence I didn't see an issue in adding it. The test projects were done for consistency but I have no issue with them being switched back if you would like.

Based on the presentation's I have seen & the training I did for a recent new starter, I don't see a file size impact by using publish single file. A difference in sizing arises when changing the publishing method of the parent project to self-contained from framework dependent. This is because framework dependent relies on the runtime from https://dotnet.microsoft.com/en-us/download/dotnet/8.0 being installed.

What I can't remember seeing/testing is if you compile a project using the base framework but it has a dependency on a project with the aspnet framework does it copy the necessary libraries to the output or is it assumed they are on the machine? This shouldn't be too hard to test. I will try and check this over the next couple of days.
Have tested this and if a project contains a package which has the Microsoft.AspNetCore.App framework reference it will not copy/publish any of the dependencies from the framework reference to the output and it will assume that the aspnet runtime is installed on the machine. If you publish as self contained it will copy the aspnet framework. I feel this approach is misleading and as such will post in dotnet repo (dotnet/sdk#38252) for clarification.

At the same time, i don't think feel that these changes introduce a new issue (might increase chances of an occurrence) but it is currently already possible for it to occur based on the fact some projects were already using package's being brought in via the framework reference.

@thompson-tomo thompson-tomo force-pushed the enhancement/#1250_CleaningOfPackages branch from 907c7a4 to 49d7859 Compare January 25, 2024 04:43
@thompson-tomo thompson-tomo force-pushed the enhancement/#1250_CleaningOfPackages branch from 49d7859 to c69608d Compare February 5, 2024 22:03
@thompson-tomo thompson-tomo force-pushed the enhancement/#1250_CleaningOfPackages branch from 15936c6 to ae457df Compare May 23, 2024 22:56
@thompson-tomo thompson-tomo force-pushed the enhancement/#1250_CleaningOfPackages branch from ae457df to b372c5c Compare May 31, 2024 09:47
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.

Clean of package references due to framework references
2 participants