aardvark-platform / aardvark.build

MIT License
0 stars 0 forks source link

ReleaseNotesTask failed in vs2022 preview #3

Open haraldsteinlechner opened 1 year ago

haraldsteinlechner commented 1 year ago

VS loads version 7 of fsharp.core in preview version which seems to result in MissingMethodExceptions when accessing paket's releasenote parsing functionality

haraldsteinlechner commented 1 year ago

seems like aardvark.build is using aardvark.build to publish itself with correct version. my fix includes using a helper library, which is not packed when using dotnet pack. the generated package is always missing the lib, even though aardvark.build has a paket.template with type file. image

krauthaufen commented 1 year ago

Maybe just run the entire code in a new AssemblyLoadContext (this way we should be sure that nothing is pre-loaded) how would an additional lib help here?

haraldsteinlechner commented 1 year ago

because the additional lib returns vanilla dotnet type (as opposed to f# list) - and it seemed to work in my repro case. packaging the thing was my problem then....

haraldsteinlechner commented 1 year ago

actually i first tried the appdomain approach, which seemed not to be available in netstandard 2.0 (depsite the docs saying the opposite). seems like AssemblyLoadContext could work also - https://gamedev.stackexchange.com/a/191579

hyazinthh commented 1 year ago

The new package requires FSharp.Core >= 6.0

Conflict detected:

  • Dependencies file requested package FSharp.Core: >= 5.0
  • Aardvark.Build 1.0.16 requested package FSharp.Core: >= 6.0
haraldsteinlechner commented 1 year ago

thanks. i unlisted the package however :/

haraldsteinlechner commented 1 year ago

the situation is still unclear.

Anyhow, having a build plugin which references a lot of stuff is not robust for the future. Also having a build time dependency fixing dependencies to particular versions is bad. So everyhing should be "statically linked", which seems impossible to do in a robust way:

EDIT: previously i reportet, wrapping the function helps. I was suspicious about it and i can confirm, my test was wrong. i still get {"Method not found: 'Microsoft.FSharp.Collections.FSharpList`1<System.String> ReleaseNotes.get_Notes()'."}

krauthaufen commented 1 year ago

I think the upshot is quite simple: you will never be able to load two different fsharp.core versions directly, so i guess there's no way around AssemblyLoadContext/AppDomain which both don't exist in netstandard... Is the msbuild plugin running in a netframework environment?

krauthaufen commented 1 year ago

Afaik msbuild is running on netframework4.8 under VisualStudio, which would be terrible since there's no AssemblyLoadContext for isolating the thing. The only other way i can think of is some command-line thing which is also sort of terrible due to serialization/platform problems/etc.

krauthaufen commented 1 year ago

Maybe just copy the Release-Notes parser and be agnostic about FSharp.Core versions?

haraldsteinlechner commented 1 year ago

i just tried to peel of release notes parsing from fake. without success, digged deeper, no idea what the real cause of the missing method exn is. the whole idea of using packages within a build task is not robust. cmd line tool is the only option

krauthaufen commented 1 year ago

Its really simple: the return type for the method cannot be resolved since the requested FsharpList from 6.0 can indeed not be found

haraldsteinlechner commented 1 year ago

no it is not. at that time hunderts of lines of f# have worked. and it is not possible to load 2 different fsharp.core's. so something is strange. also i cannot reproduce it anymore

haraldsteinlechner commented 1 year ago

execution runs till here: image Notes is not found, although the runtime type has a get_Notes.

krauthaufen commented 1 year ago

That is still how it is, most of fsharp.core is backwards compatible, so 90% of things just work, but the list used for compiling Fake.ReleaseNotes seems to be incompatible with the one from your Fsharp.core That doesn't necessarily mean different, its just some tokens/etc being different and that is precisely what happens.

Without our "accept every lib having the correct name" loading stuff your code would explode way earlier, so this is danger zone

haraldsteinlechner commented 1 year ago

so it is the FSharp.Core >= which causes the problem at some point.

krauthaufen commented 1 year ago

No I install custom assembly-loading stuff in the plugin afaik to simply ignore version mismatches which made the plugin work in older versions, but obviously this no longer helps. Basically there is no other option than an isolated environment for the whole thing

haraldsteinlechner commented 1 year ago

i did not know about this one ;) just found it in Utilities. So in the end we need to create a cmd line tool - and invoke it just like paket.

krauthaufen commented 1 year ago

Most likely yes, but that is a pain in the XXX since it needs to work on all platforms without opening terminal windows, etc.

haraldsteinlechner commented 1 year ago

i just linked fsharp.core statically, mono and sharpziplib are hopefully unproblematic, just trying to get a version out. What makes me wonder is that old aardvark.build versions had an empty dependency list - builds generated using pack.cmd have dependencies. i assume no dependencies is the right way

krauthaufen commented 1 year ago

Hey, since this is a build-only thing it shouldn't give you any transitive dependencies... So having no deps is actually quite important

haraldsteinlechner commented 1 year ago

i went the "let us just parse that little file ourselfs" path. when we have better solutions for isolated environments (or static link bugs fixed), we could switch to more robust FAKE again. I just wanted to make it work for all people being blocked by annoying failures.

PS.: i had sever problems publishing the package correctly. i switched to a getversion.fsproj and .cmd file to use paket pack which makes it work. dotnet pack seems to ignore the template file, aardpack currently still fails if no vs2019 is installed (see other issue)

hyazinthh commented 1 year ago

Parsing the release notes file ourselves also means that we can add support for comments. Would be useful for adding release notes without actually deploying a new package. Currently, you have to go over all commits and add them to the notes when you want to deploy a new version. This is cumbersome and may also lead to incomplete release notes if someone builds a package without doing that extra step.

@luithefirst suggested this at some point IIRC.

haraldsteinlechner commented 1 year ago

would be cool but this moves us away from standard release notes files. wouldn't it be simpler to just write a current_release_notes.md which tracks the changes, and finally when done it is copied into the real release_notes?

krauthaufen commented 1 year ago

The natural way would be prepending bullet-points without a version-heading? I think that should be fairly easy to ignore in the parser. Essentially this could be done by skipping everything before the first # at the beginning of a line.

haraldsteinlechner commented 1 year ago

the PR includes the final fix (without this feature request). please check if readme.md is appropriate in the PR https://github.com/aardvark-platform/aardvark.build/pull/6