-
Notifications
You must be signed in to change notification settings - Fork 301
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
A word about ProjectCracker #3613
Comments
Yep, this makes sense to me. A couple notes:
|
Did a quick local test:
When adding: <ItemGroup>
<PackageReference Include="Fable.Core" Version="4.2.0" />
<PackageReference Include="Thoth.Json" Version="13.0.0">
<ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
</PackageReference>
<PackageReference Include="FableLibB" Version="1.0.0">
<ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
</PackageReference>
<PackageReference Include="FableLibC" Version="1.0.0">
<ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
</PackageReference>
</ItemGroup> The files of The order I added |
@baronfel turns out you can put the |
If I understand correctly, it means that the user needs to write: <PackageReference Include="Thoth.Json" Version="13.0.0">
<ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets>
</PackageReference> in their How does this works with paket? We also need to check that this works with MSBuild central package management. |
Yes, although we can drop the
It works with Paket, except that you cannot do the
I don't see any problem there as that merely controls the version of the Overall, I'm still optimistic about this idea. Not doing the PS: A better MSBuild way to construct the source file path:
<Project>
<ItemGroup Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">
<CompileBefore Include="$([MSBuild]::NormalizePath($(MSBuildThisFileDirectory), '../fable/FableLibA.fs'))"/>
</ItemGroup>
</Project>
|
Personally, I like that more as all his hidden for the end user. From experience, the more streamline things are the better. Another question I have in regard to: <ExcludeAssets Condition="$(DefineConstants.Contains('FABLE_COMPILER'))">compile</ExcludeAssets> is what does the
Having fable-standalone figures things out in another way is ok to me, however we do need to have a solution for fable-standalone because this is used by Fable REPL and this is a really useful tool for prototype and most importantly bug reports too. If we are able to find the |
If we are going down this road, we will also need to think how we want the transition period to be done.
I also believe we should consider retrieving the Fable packages from NuGet kind of like https://fable.io/packages does, and test for libraries that includes |
"Contents of the lib folder and controls whether your project can compile against the assemblies within the folder"
Yeah, I agree it should definitely still work. We should find a good way to extract where standalone will do its own resolution.
Yup, I would also assume having a Fable 5 would allow us to address some opportunities that benefit from making a breaking change. |
As already mentioned, I think #3549 has some overlapping goals. I was already mulling over potential breaking changes in package structure that would help it - we could take this as an opportunity to rethink packages a little bit. A custom TargetFramework would get rid of that conditional ugliness that @nojaf is having to use right now. It would be completely transparent to the Fable user. I like the idea of doing this for Fable 5, and adding some forward compatibility to Fable 4 depending on how this shapes up |
Hello, as I'm exploring some options to achieve #3552, I'm also learning about the internals of ProjectCracker.
I knew Fable needed the F# source files in certain scenarios to compile NuGet dependencies.
The documentation mentions putting all source files in the
fable
PackagePath
together with afsproj
.getFullProjectOpts appears to be the entry point of doing the actual cracking. It will crack the main fsproj and detect fable libraries among the references in tryGetFablePackage.
This code really goes and scans the extracted NuGet package in search of a
fable
folder. Find thefsproj
and collect the source files.Let's imagine we are using
Thoth.Json
in our project.To construct the FSharpProjectOptions, Fable will inject the source files from
Thoth.Json
There is a lot of custom logic involved to pull this off, and I'm wondering if we shouldn't just embrace MSBuild a little more in this case.
Locally I created
Thoth.Json v13
with the following change:If the NuGet package were to add a
build/<package-id>.props
file, it would be very convenient to include the source files to the actualfsproj
:You really only want to include the source files when Fable is evaluating the project. Inside the IDE you still want to rely on the binary to get IntelliSense.
Inside my
fsproj
I would addThoth.Json
like this:Alright, to see the results of this in action:
Running
dotnet build -bl /p:DefineConstants=FABLE_COMPILER --no-incremental -v:n
shows us the followingCoreCompile
output:I'm including the source files for
Thoth.Json
before my own code.And I don't have the
-r:Thoth.Json.dll
.When running without
/p:DefineConstants=FABLE_COMPILER
I get:Notice I don't have the
Thoth.Json
source files but I do have the-r:C:\Users\nojaf\.nuget\packages\thoth.json\13.0.0\lib\netstandard2.0\Thoth.Json.dll
.This moves a lot of logic from
ProjectCracker
toMSBuild
which is good at these sorts of project system evaluations.I'm quite sure this would be beneficial for @jwosty in #3549.
As of right now, this is a limitation to piggyback on MSBuild.
Going in this direction would of course be a breaking change to the Fable ecosystem. However, it opens the door to having a better grasp on project options extraction. This would allow us to troubleshoot things easily from binary logs.
The breaking changes on the author side would be to include a
props
file and on the consumer side toExcludeAssets compile
for Fable packages.I'm assuming someone will make the argument that
fable-standalone
probably can't rely on this. That may be a fair critique, however, I don't think it stands. The gain of usingMSBuild
is significant for what I expect to be 95% of Fable's current use case, so I think it makes sense thatfable-standalone
figures things out in another way.@baronfel am I making sense here MSBuild-wise?
The text was updated successfully, but these errors were encountered: