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

TEST: Parse projects as .csproj #3265

Merged
merged 3 commits into from
Nov 12, 2022
Merged

TEST: Parse projects as .csproj #3265

merged 3 commits into from
Nov 12, 2022

Conversation

alfonsogarciacaro
Copy link
Member

@alfonsogarciacaro alfonsogarciacaro commented Nov 12, 2022

It just crossed my mind that project parsing is almost the same in F# and C#, but the latter is going to be usually better supported so, what if we parse the project as if it were a C# one and tweak the results? In this PR I just copy the .fsproj into a .csproj file, pass it to Buildalyzer and with a few adjustments it seems to work. One of the main differences is the F# compiler inserts FSharp.Core when it's not explicitly referenced, but this is already done when restoring.

Another important advantage is Buildalyzer can do a design-time build for C# projects which is much faster. For F# projects parsing takes as long as a normal build (I experienced the same with Dotnet.ProjInfo and Ionide.ProjInfo).

All the build jobs are working except for Python. @dbrattli Could you please have a look at the logs? But if the error message doesn't ring a bell immediately don't worry and I'll try to debug the Python build and compare the compiler args when parsing .fsproj or (fake) .csproj.

What do you think? Looks like a viable option or is too hacky? cc @ncave

@dbrattli
Copy link
Collaborator

@alfonsogarciacaro I think this must be releated to if Python thinks the project is an executable (non-relative imports) or a library (relative imports). The test project should be an executable so somehow the OutputType is not detected (correctly) anymore. I'll try to run locally and see if I can spot the problem.

@alfonsogarciacaro
Copy link
Member Author

Ah, you're right, thanks! This should be easy to fix, I'll have a look.

@dbrattli
Copy link
Collaborator

@alfonsogarciacaro, but default should be "Library" and the test project file for Python does not set it. If you add <OutputType>Exe</OutputType> to Fable.Tests.Python.fsproj then it works again. How did this ever work before? 🤔

@dbrattli
Copy link
Collaborator

@alfonsogarciacaro I wonder if using e.g <GenerateProgramFile>false</GenerateProgramFile> flips it into an executable. I'm not sure this option is available in csproj files?

@dbrattli
Copy link
Collaborator

Or is fsharp test projects by default executable? Ref: https://stackoverflow.com/questions/66941970/but-it-is-entrypointattribute-attribute-must-be-the-last-declaration-in-the-l

"A 'Program.fs' file can be automatically generated for F# .NET Core test projects. To fix this warning, either delete the file from the project, or set the property to 'false'.."

@alfonsogarciacaro
Copy link
Member Author

You're right @dbrattli. It seems the problem is when compiling as a .csproj the default for OutputType is different. I'm also overriding TargetFramerwork as net6.0 which may have also an effect.

If we set OutputType explicitly the problem is solved, but not sure if this is an acceptable solution for you? I guess the main problem in breaking existing projects. Assuming that we expect most of Fable.Python users to compile projects as executables, maybe when we detect it's compiled as Library we can display a message like:

"Compiling project as Library. This is normally used if you want to distribute the code as a Python package. If you intend to run the code directly, please set OutputType to Exe."

@dbrattli
Copy link
Collaborator

@alfonsogarciacaro, I think this should be fine. It will perhaps break a few test projects, but it's sort of confusing that it actually did work without setting output type to Exe. It's such a shame that the import system of Python is so confusing 🤦 No-one really understands it fully. As for the message we could simplify it e.g: "Compiling project as Library. If you intend to run the code directly, please set OutputType to Exe.". Because we do not usually distribute the library as a package when using Fable, we just import it as a module (library) into an existing program e.g like we do with fable_library.

@alfonsogarciacaro
Copy link
Member Author

Ok, let's merge this, publish and give it a try. If it gives problems we can always revert the PR.

@ncave
Copy link
Collaborator

ncave commented Nov 13, 2022

It's such a shame that the import system of Python is so confusing 🤦 No-one really understands it fully.

@dbrattli You should check out the import system in Rust then, it may change your mind about Python's 🙂

I would consider not having to use it a major selling point for Fable just by itself.

@ncave
Copy link
Collaborator

ncave commented Nov 14, 2022

@alfonsogarciacaro From what I've seen trying to use Buildalyzer with net7.0 projects, the references are not extracted from .fsproj, but with .csproj it works properly. So yeah, a good change.

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.

3 participants