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

Prepared new package versions 1.2 #51

Merged
merged 3 commits into from
Jan 31, 2023
Merged

Prepared new package versions 1.2 #51

merged 3 commits into from
Jan 31, 2023

Conversation

rivantsov
Copy link
Contributor

  1. Switched Irony, Irony.Interpreter packages to netStandard2.0 (removed target Net4.0); .NET standard 2.0 is up compatible with everything
  2. Switched GE and tests to .NET 4.7.2
  3. Upped version to 1.2, rebuilt, prepared push to nuget

@rivantsov rivantsov requested a review from yallie January 31, 2023 18:45
@yallie
Copy link
Member

yallie commented Jan 31, 2023

Hi Roman, thanks for the PR! 👍

Can we keep 4.x framework targets in Irony and Irony.Interpreter, maybe net46?

Although netstandard 2.0 target is supposed to be compatible with 4.x frameworks,
it adds weird netstandard reference to the assemblies:

image

I've had issues with nuget packages targeting only netstandard 2.0 framework in the past.
Something like this error message: You must add a reference to assembly 'netstandard, Version=2.0.0.0'.

Many modern System packages still target .NET 4.x frameworks explicitly, see

Open-source projects maintaining backwards compatibility also tend to target 4.x frameworks explicitly, see

  • linq2db: net45 + netstandard + net60
  • DryIoc: net45 + netstandard,

to name a few.

So, can we also please keep net46 target as well?

@rivantsov
Copy link
Contributor Author

Let me look at it. But for now I have some questions

  1. I don't get this: "... it adds weird netstandard reference to the assemblies" with the following pic. What's so weird there? maybe I am missing some knowledge about internals of IL, but pls enlighten me.
  2. The error message and link to StackOverflow. Isn't the problem that the main app's target framework is Net 4.6.1; while I believe 4.6.2 is the first one compatible with Net Standard 2.0?

I see a trouble with including very old fwks, because it forces me to install this old crap just to build the project. This is really annoying and the thing that irritates me a lot when I open projects from github. Plus it doubles/triples the size of packages with stuff that almost no one ever uses.

In general, I do not mind adding 4.6.2 target, but I believe most common out there is 4.7.2, while 4.8 is where people chose to switch to Net 5 or 6

@rivantsov
Copy link
Contributor Author

About this argument:

Many modern System packages still target .NET 4.x frameworks explicitly

and packages list: Buffers, ValueTuple, CompilerServices. I do not think this example applies here.

First, for historical reasons, these have to maintain version for oldest frameworks, when netStandard wasn't there. So that already compiled stuff in prod continues to work without rebuild (or with rebuild in case of security patch but without retargeting which is extra risk). No retargeting - that's very fundamental requirement for this kind of low-level foundational system libraries.

Secondly, I think (guess) this functionality (buffers, value tuples) is close-to-metal, needs extreme optimization, and I guess they have all kinds of tricks and conditional "#if NET_VERSION = ?" branches to use optimal tricks for specific Fwk version.

None of this applies to Irony - I think, we are not foundational (used by whole world in prod for 10 years without rebuild), or low-level close to metal - it is all just regular high-level operations.

This is only about one specific argument; I am ready to listen to counter-arguments. I do understand that if it causes other troubles like using/compiling in "regular" projects in not so old Visual Studio, then it is very valid concern. So far my personal experience with .NET standard 2.0 was great, and I try to stay with it as far as I can, that explains this change. I can be wrong in this case

@rivantsov
Copy link
Contributor Author

One more thing - I tried to switch all projects to new SDK-style format (short xml), succeeded for most except Irony.WinForms, something did not work at all and I quickly gave up. And test projects. Would be great if we can get rid of this old csproj ugliness. thanks

@yallie
Copy link
Member

yallie commented Jan 31, 2023

Hi Roman, thanks for your replies.

I don't get this: "... it adds weird netstandard reference to the assemblies" with the following pic. What's so weird there?
maybe I am missing some knowledge about internals of IL, but pls enlighten me.

There is no way I can enlighten you Roman, my knowledge is very limited. I can share my observations at most.

When the project targets netstandard 2.0, the built assembly includes the reference to "netstandard" assembly which is missing in net4x frameworks, as seen on my screenshot.

As far as I understand, "netstandard" is not a real assembly, but a synonym for a bunch of standard libraries like mscorlib, System, System.Core, etc. Older build tools don't understand this convention and give this "You must add a reference" error message.

while I believe 4.6.2 is the first one compatible with Net Standard 2.0?

Yes, that's possible. I work with a bunch of projects targeting net461, so it may be the reason I've seen this error message before. I was unaware that netstandard 2.0 is not supposed to be compatible with net461.

I see a trouble with including very old fwks, because it forces me to install this old crap just to build the project. This is really annoying and the thing that irritates me a lot when I open projects from github.

That sounds reasonable. I myself tend to remove targets I don't need when building someone else's projects from github to avoid installing outdated targeting packs.

Plus it doubles/triples the size of packages with stuff that almost no one ever uses.

Agree.

In general, I do not mind adding 4.6.2 target, but I believe most common out there is 4.7.2, while 4.8 is where people chose to switch to Net 5 or 6

You're probably right, net472 may be most common out there. I just still need to work with net461 on some projects. But I'll do just fine without updating to the latest version of Irony if keeping net46 target is problematic.

and packages list: Buffers, ValueTuple, CompilerServices. I do not think this example applies here.

My point was that many modern packages still target net46x explicitly. If netstandard 2.0 target were 100% compatible with net46x, what would be the reason for that? I think that full backwards compatibility is the reason.

So that already compiled stuff in prod continues to work without rebuild

Exactly.

Alright, I don't have any better arguments except the one I mentioned before. I would prefer keeping net46 target for compatibility reasons, but if it bothers you Roman, I don't insist at all. Your argument about installing the old crap never came to my mind. My laptops seem to be old enough to have all .NET SDKs since v4.5 already installed.

Cheers!

@rivantsov
Copy link
Contributor Author

Ok, I suggest to merge, I already pushed the new packages with netstandard only; let's see if anybody starts screaming, if yes, we'll add 462 or 472 back.
About netstandard, it is an 'interface' library, it contains only declarations of methods or types, not implementations, with a promise that actual implementations will be in supporting versions, so netstandard 2.0 is supported in 462 (problems in 461):

https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0

there is also a forward promise that all current and future net (core) versions (5, 6, 7 etc) will implement it. So it is a very safe target. I will create an issue asking if everybody is ok with netstandard only.
thanks!

@rivantsov rivantsov merged commit b8330ef into master Jan 31, 2023
@rivantsov rivantsov deleted the roman_dev branch March 10, 2024 19:24
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.

2 participants