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

Publish AOT artefacts for Fantomas #3090

Draft
wants to merge 28 commits into
base: v7.0
Choose a base branch
from

Conversation

TobyShaw
Copy link

Please verify your pull request is respecting our Pull request ground rules.
You can find more information in our Contributors documentation section.

@TobyShaw
Copy link
Author

Currently getting:

root@15970d89aedc:/workspaces/fantomas/src/Fantomas# ../../artifacts/publish/Fantomas/release_linux-x64/fantomas
System.TypeLoadException: Attempted to load a type that was not created during ahead of time compilation.
   at Internal.Runtime.CompilerHelpers.ThrowHelpers.ThrowUnavailableType() + 0x27
   at Microsoft.FSharp.Core.FSharpFunc`2.InvokeFast[V](FSharpFunc`2, T, TResult) + 0x1e
   at Fargo.Fargo.map2@353.Invoke(FSharpList`1) + 0x8a
   at Fargo.Fargo.map2@353.Invoke(FSharpList`1) + 0x57
   at Fargo.Fargo.map2@353.Invoke(FSharpList`1) + 0x57

@Numpsy I saw you mentioned that Fargo was AOT compatible. Did you run into this issue when testing this?

@TobyShaw TobyShaw mentioned this pull request May 31, 2024
4 tasks
@Numpsy
Copy link

Numpsy commented May 31, 2024

I have a CLI tool in a project at work that's working with Fargo in AOT.

Do you have any AOT analysis warnings in the build that suggest issues?

@Numpsy
Copy link

Numpsy commented May 31, 2024

Actually, looks like it might be dotnet/fsharp#15488 (reported by the author of Fargo)?

@TobyShaw
Copy link
Author

TobyShaw commented Jun 1, 2024

That was it, bingo!
Thanks very much for tracking that down!

@TobyShaw
Copy link
Author

@nojaf Do you know why the builds aren't completing? I'd like to get to all green ticks before swapping the PR to be non-draft.

@nojaf
Copy link
Contributor

nojaf commented Jun 10, 2024

I need to press a button to approve running CI first. I believe this is because it is your first contribution.

@nojaf
Copy link
Contributor

nojaf commented Jun 10, 2024

You might also want to sync your PR with the latest main.

@TobyShaw
Copy link
Author

Thanks!

@nojaf
Copy link
Contributor

nojaf commented Jul 8, 2024

If the lock file is too much of a pain, I think I can live without them.

@TobyShaw
Copy link
Author

TobyShaw commented Jul 8, 2024

I think I've solved the lockfile issue, I was mostly just getting confused as I accidentally left a stray RuntimeIdentifier in Fantomas.fsproj.

@TobyShaw
Copy link
Author

TobyShaw commented Jul 8, 2024

@nojaf I see that my build is succeeding in the dev container ( https://github.com/fsprojects/fantomas/actions/runs/9841310391/job/27168319253) but not on the other platforms (https://github.com/fsprojects/fantomas/actions/runs/9841310391/job/27168319712)

/home/runner/work/fantomas/fantomas/src/Fantomas/Fantomas.fsproj : error NU1004: The package reference
Microsoft.DotNet.ILCompiler version has changed from [8.0.2, ) to [8.0.6, ).The packages lock file is inconsistent with the project 
dependencies so restore can't be run in locked mode. Disable the RestoreLockedMode MSBuild property or pass an explicit --
force-evaluate option to run restore to update the lock file. [/home/runner/work/fantomas/fantomas/fantomas.sln]
    0 Warning(s)
    1 Error(s)

Do you have an idea what's different between the dev container vs the other platforms, and how to fix this?

@baronfel
Copy link
Contributor

baronfel commented Jul 8, 2024

If you use lockfiles then you'll need to pin the SDK version very strictly (no floating at all) to ensure reproducibility. The AOT support packages bump with almost every release of the SDK (including monthly security releases) and so are very up for change.

The lockfile mechanism in NuGet doesn't easily allow for ignoring these kind of sdk-provided packages, and it's an area of pain that the SDK team is trying to bridge with the NuGet team.

So right now, either:

  • use global.json to pin to a single sdk version across contributors, maintainers, and CI/CD and continue to use lockfiles
  • stop using lockfiles

@Numpsy
Copy link

Numpsy commented Jul 8, 2024

Do you have an idea what's different between the dev container vs the other platforms, and how to fix this?

does the dev container only have the 8.0.200 SDK installed, and the other runners have that plus 8.0.3XX ?

@TobyShaw
Copy link
Author

TobyShaw commented Jul 8, 2024

If the lock file is too much of a pain, I think I can live without them.

Thinking about this more, how about they're kept for the main pipeline but we pass -p:RestoreLockedMode=false to the AOT-publish pipeline? I'd like to avoid muddying the main repo with this publish method, mostly because AOT seems not very well supported in the .NET ecosystem yet.

@nojaf
Copy link
Contributor

nojaf commented Jul 9, 2024

Thinking about this more, how about they're kept for the main pipeline but we pass -p:RestoreLockedMode=false to the AOT-publish pipeline?

Would work for me.

does the dev container only have the 8.0.200 SDK installed, and the other runners have that plus 8.0.3XX ?

Yes, taking a look at this in #3100

@nojaf
Copy link
Contributor

nojaf commented Jul 10, 2024

@TobyShaw could you rebase (or merge master) again?

@TobyShaw
Copy link
Author

Sorry for commit spam, having to edit via the browser lol

@nojaf nojaf changed the base branch from main to v7.0 September 16, 2024 12:46
@nojaf
Copy link
Contributor

nojaf commented Sep 16, 2024

Hi @TobyShaw, I've changed the base branch to v7.
Please target that branch going forward. We can make breaking changes there if necessary.

@nojaf
Copy link
Contributor

nojaf commented Nov 18, 2024

Hello @TobyShaw, do you have any plans to pick this up in the near future?
I'm asking if we need to wait to release Fantomas v7 for this feature.

@TobyShaw
Copy link
Author

TobyShaw commented Nov 26, 2024 via email

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.

4 participants