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

net461 and netstandard2.0 with working evaluation #480

Merged
merged 18 commits into from Oct 22, 2018

Conversation

thinkbeforecoding
Copy link
Contributor

This version prepares FSharp.Literate to work on netstandard2.0 with evaluation to be used from netcoreapp projects.
The net461 version is still built with command line tools

@wallymathieu
Copy link
Member

Cool! I hope it goes well with the PR!

@thinkbeforecoding
Copy link
Contributor Author

I have a problem with travis:

The current .NET SDK does not support targeting .NET Core 2.1. Either target .NET Core 2.0 or lower, or use a version of the .NET SDK that supports .NET Core 2.1.

Even if the .travis.yml specifies dotnet : 2.1.4

@thinkbeforecoding
Copy link
Contributor Author

An appveyor is failing with an error that I managed to fix local by removing paket-files\paket.restore.cached and running paket restore again…
But there is probably a better way to not get this error.

@matthid
Copy link
Member

matthid commented Oct 14, 2018

An appveyor is failing with an error that I managed to fix local by removing paket-files\paket.restore.cached and running paket restore again…
But there is probably a better way to not get this error.

Add that file to gitignore and do not commit it.

Even if the .travis.yml specifies dotnet : 2.1.4

Try with latest 2.1.403 or something like that. If that doesn't work I'd assume something in the build still uses an older version.

@thinkbeforecoding
Copy link
Contributor Author

The directory paket-file is already in .gitignore, is appveyor working on a clean directory when launching build, or does it live the directory as is between builds ?

@matthid
Copy link
Member

matthid commented Oct 14, 2018

appveyor should use a new machine every time (so it should be clean)

@thinkbeforecoding
Copy link
Contributor Author

I'm making new attempts 👍

@thinkbeforecoding
Copy link
Contributor Author

Now it's complaining about net461.
The build is working fine on windows, but on mono I get:

The reference assemblies for framework ".NETFramework,Version=v4.6.1" were not found. To resolve this, install the SDK or Targeting Pack for this framework version or retarget your application to a version of the framework for which you have the SDK or Targeting Pack installed. Note that assemblies will be resolved from the Global Assembly Cache (GAC) and will be used in place of reference assemblies. Therefore your assembly may not be correctly targeted for the framework you intend.

@wallymathieu
Copy link
Member

Sometimes I've had some success with using what has been done in the type providers: Working on dotnet core on Mac OS X part 2

@matthid
Copy link
Member

matthid commented Oct 14, 2018

@thinkbeforecoding You might want to add the reference assembly nuget package nuget RoslynTools.ReferenceAssemblies which can be found in the feed source https://dotnet.myget.org/F/roslyn-tools/api/v3/index.json I have already added that for similar reasons to other repositories (like FAKE).

For this to work you need to edit/add a directory.props file: https://github.com/fsharp/FAKE/blob/694f616c97fa242162cfd36db905d7df3156018f/src/Directory.Build.props#L3

@thinkbeforecoding
Copy link
Contributor Author

Trying this.

@thinkbeforecoding
Copy link
Contributor Author

paket fails while trying to load roslyntools… any idea ?

@matthid
Copy link
Member

matthid commented Oct 14, 2018

@thinkbeforecoding did you add the source? It is sadly not available on official nuget

@thinkbeforecoding
Copy link
Contributor Author

yes I added the source and paket install is working..
But paket restore takes ages and fails.

@thinkbeforecoding
Copy link
Contributor Author

@matthid added the reference assemblies and the Directory.Build.props and it's still failing with the same error.

@thinkbeforecoding
Copy link
Contributor Author

I think I had the relative path wrong. new try..

@thinkbeforecoding
Copy link
Contributor Author

There is some progress, now I have a problem with app.config files in tests.

@thinkbeforecoding
Copy link
Contributor Author

This error also occurs only in mono. I managed to reproduce it using WSL bash for windows, but still don't know why this happens .

@thinkbeforecoding
Copy link
Contributor Author

The build passed on appveyor, but I could not find testresult.xml due to the fact that it's now using dotnet test and not nunit console runner.
have to see what I do with this...

@thinkbeforecoding
Copy link
Contributor Author

I have a problem reproducing the build locally because from paris downloading oslynTools.ReferenceAssemblies takes 7min and timeouting 😢

@wallymathieu
Copy link
Member

Do you still need to keep app config around with the new style format?

@thinkbeforecoding
Copy link
Contributor Author

Yes for the test project running in net461, they are needed for binding redirects.

When removed, a resolution of an FSharp.Core 4.1.0.0 is failing.

@thinkbeforecoding
Copy link
Contributor Author

@wallymathieu the appveyor build is queued but doesn't seem to start...

@wallymathieu
Copy link
Member

It will probably queue up a new build once it gets available. I can imagine that fsprojects use quite a lot of appveyor resources.

@thinkbeforecoding
Copy link
Contributor Author

It has been rather quick today, but there are probably changes on other repos as well...

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments

FSharp.Formatting.TestHelpers/TestHelpers.fs Outdated Show resolved Hide resolved
nuget/FSharp.Formatting.nuspec Show resolved Hide resolved
@thinkbeforecoding
Copy link
Contributor Author

The build finally passed on appveyor

@thinkbeforecoding
Copy link
Contributor Author

Anything more ?

Copy link
Member

@wallymathieu wallymathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@wallymathieu
Copy link
Member

I'll wait on @matthid since this is such a big PR.

@thinkbeforecoding
Copy link
Contributor Author

@matthid when do you think you'll have time for the review ? 😄 I can help if needed..

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this update the "publishing" logic in the fake script to push thr the new package?
Is the new package sent to the appveyor feed?

appveyor.yml Show resolved Hide resolved
build.fsx Outdated
Arguments = sprintf "%s --fsiargs -d:FAKE %s \"%s\"" args fsiargs script
WorkingDirectory = workingDirectory
}
|> Process.withFramework
|> Process.setEnvironmentVariable "MSBuild" MSBuild.msBuildExe
// |> Process.setEnvironmentVariable "MSBuild" MSBuild.msBuildExe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes dotnet is already doing it properly

build.fsx Outdated
@@ -537,42 +536,42 @@ Target.create"CreateTag" (fun _ ->
Git.Branches.pushTag "" "origin" release.NugetVersion
)

Target.create"Release" Target.DoNothing
Target.create"Release" ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a space after create :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup 😄

@thinkbeforecoding
Copy link
Contributor Author

Done 👍

@matthid
Copy link
Member

matthid commented Oct 18, 2018

Overall looks good, but what about the publishing of the new nuget? did you add that code or did I just miss it?

@thinkbeforecoding
Copy link
Contributor Author

I've not seen where it's done :grin

@thinkbeforecoding
Copy link
Contributor Author

The nuget is created in the folder, but I've not seen where it's pushed to nuget.org ..

@thinkbeforecoding
Copy link
Contributor Author

Ah ! Actually the nuget/publish.cmd published all the bin/*.nupkg so it's good 👍

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets see what happens, thanks a lot!

@thinkbeforecoding
Copy link
Contributor Author

great 😄

@matthid matthid merged commit d8be726 into fsprojects:master Oct 22, 2018
@thinkbeforecoding
Copy link
Contributor Author

Thx !
Where are the nuget published ?

@thinkbeforecoding
Copy link
Contributor Author

Ok 👍🏻 👍🏻

@matthid matthid mentioned this pull request Apr 10, 2019
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.

None yet

3 participants