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

[Ready] Make paket build on a clean windows #2664

Merged
merged 5 commits into from
Sep 3, 2017

Conversation

0x53A
Copy link
Contributor

@0x53A 0x53A commented Aug 25, 2017

Windows

  • Install Windows 10 Pro from ISO
  • install git
  • clone paket
  • run build.cmd

image
(Note: IntegrationTests were skipped in this screenshot (because they take so much time), but they were also green!

Linux

We still rely on a global mono and a global MSBuild, because the bundled version is not compatible with linux and crashes hard on startup.

Notes:

  • msbuild is provided by 'RoslynTools.MSBuild'
  • ReferenceAssemblies are currently checked-in. I need to either find a nuget package, or create one myself. (I had planned to use 'RoslynTools.ReferenceAssemblies', but that doesn't contain the ones for 4.5) I created a nuget package using https://github.com/jaredpar/xcopy-msbuild. This contains only the two directories it needs (NETFramework\v4.5 to compile against net45 AND .NETPortable\v4.5 to consume portable assemblies, for example FsCheck)
  • MSBuild doesn't seem to support Directory.build.props in combination with old-sdk projects? I had wanted to use that to set FrameworkPathOverride, but it didn't work until I manually modified all fsprojs Only the first Directory.Build.props is used, and there was already one in /src/. Also, Directory.build.targets does not yet work: Directory.build.targets not imported when cross-targeting dotnet/msbuild#1721
  • there is something funky with referencing portable stuff, I had to disable the FsCheck tests, or I would get the following errors: fixed after adding ref-assembly directory .NETPortable\v4.5

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.

Is there another way than to add all those binaries to the repository?

@0x53A
Copy link
Contributor Author

0x53A commented Aug 25, 2017

what binaries do you mean? ;D

@@ -44,3 +44,9 @@ group Build
nuget ILRepack

github fsharp/FAKE modules/Octokit/Octokit.fsx

nuget 0x53A.ReferenceAssemblies.net45 0.1.0
Copy link
Member

Choose a reason for hiding this comment

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

So this is for net45 support, correct? because in the future this is no longer required as soon as we are fine with net461 compat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, these are the reference assemblies. When we move to net461 we need to update to the correct reference assemblies for that.

The reference assemblies are the "contract assemblies" that the compiler compiles against. They are metadata only.

I used https://github.com/jaredpar/xcopy-msbuild, this basically just zipped up the content of C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.5

Copy link
Member

Choose a reason for hiding this comment

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

But once we are on net461 all we need is the netstandard build, so we could drop that in theory?
I don't think this is the correct thing to do long term (though I haven't really decided yet).

I thought we will have all the reference assemblies as "official" nuget packages now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't found any reference assemblies as nuget packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise I'm pretty sure the roslyn build would use these, instead of packaging this stuff up themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate/link the RoslynTools.ReferenceAssemblies you are referring to above (I cannot find it)?

source https://dotnet.myget.org/F/roslyn-tools/api/v3/index.json

And also why it doesn't work for us (just trying to figure out if we can make it work or if it is viable for other scenarios)?

It does not contain the ref-assemblies for 4.5:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, maybe I can use this?
image
Will try it ...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe should just upgrade to net46? I'm not sure about the portable stuff could be a subset..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I did a test, and I am pretty sure we need .NETFramework\v4.5 to compile against net45 AND .NETPortable\v4.5 to consume portable assemblies, for example FsCheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a package that contains only these two. Reason: it is smaller.

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

The tool dotnet-mergenupkg was compiled against netcore1.0, so we need the netcore1.0.x runtime to execute it ...

The issue is that Fake only allows the installation of a single runtime, I tried installing two after each other but the second deleted the first.

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

Hi @enricosada, do you think it would be possible to update dotnet-mergenupkg and provide a netcoreapp2.0 binary?

Otherwise usage in the Sdk 2.x is a bit clumsy, because I still need to install the 1.0.x runtime in parallel somehow.

I've hacked together a netcoreapp2 version, so I can continue testing this: enricosada/dotnet-mergenupkg@master...0x53A:master

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

image

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.

Way better :). I like the direction of this - It probably will make contributing easier.

@@ -44,3 +44,10 @@ group Build
nuget ILRepack

github fsharp/FAKE modules/Octokit/Octokit.fsx

nuget 0x53A.ReferenceAssemblies.Paket 0.2.0
Copy link
Member

Choose a reason for hiding this comment

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

So every project now bundles their own reference assemblies :P

Copy link
Member

Choose a reason for hiding this comment

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

Or put differently, if you think this is a good way forward (which I think it is). We might want to have another documented project on how this stuff works (Then others can use it and read about it). Anyway that's just a suggestion (I'd like to maybe add that to FAKE as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I would prefer to use an official package, also.

The one advantage this has is that it only bundles the directories it actually needs, so the size is a bit smaller. I could have given it a specific name like "ReferenceAssemblies.Net45", but this way it is clear that it is used by paket and will change as soon as we update.

Building the package is dead easy, clone https://github.com/jaredpar/xcopy-msbuild, modify which frameworks to include https://github.com/jaredpar/xcopy-msbuild/blob/006e1bb65d031854f38eaac251b5f799ce281e37/build-reference-assemblies.ps1#L23-L30
and run the powershell script.

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 should add that as comment then?

build.cmd Outdated
@@ -5,4 +5,7 @@ if errorlevel 1 (
exit /b %errorlevel%
)

set MSBUILD=packages\build\RoslynTools.MSBuild\tools\msbuild
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 could set that in the FAKE script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, that may also fix unix, which currently fails (because it still runs the bundled msbuild).

How would you set it inside Fake? Just set the env vars? or is there some config switch?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I think in FAKE 4 the way would be to change the mutable field with the path.
But yeah I don't know how a good FAKE API for this would look like...

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

Well, i guess I need to set up a linux vm to fix this :\

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

what a bunch of crap:

image

@matthid
Copy link
Member

matthid commented Aug 26, 2017

Oh yeah ;)

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

well, one issue was that ubuntu 17.x still references some ancient 4.x mono ...

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

does paket not use caching on unix?

Maybe I'm wrong ,but it seems to download everything every time

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

wait - paket now finished successfully???????

I only had to start the build script 20 times!

@forki
Copy link
Member

forki commented Aug 26, 2017 via email

@matthid
Copy link
Member

matthid commented Aug 26, 2017

Well, if we don't find a global cache we fallback to some local directory. Maybe that is what you are seeing?

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

Idk, I was probably confused. Each time I started it, it downloaded some, then failed. But each time it probably got a little bit further...

You may want to mute this thread, i will probably rage a bit until I either have this fixed, or given up ;)

@forki
Copy link
Member

forki commented Aug 26, 2017 via email

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

good news: I got it to use the msbuild from the package.

bad news: it crashes hard on startup

image

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

Ready from my side, as soon as AppVeyor and at least one travis leg are green

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.

I'd love to see a bit less "WIP"-Dependencies but overall that's a good step forward.

@@ -44,3 +44,10 @@ group Build
nuget ILRepack

github fsharp/FAKE modules/Octokit/Octokit.fsx

nuget 0x53A.ReferenceAssemblies.Paket 0.2.0
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 should add that as comment then?

@@ -1,9 +1,12 @@
<Project>
<!-- MSBuild only includes the first Directory.Build.props, so we need to manually include the root one -->
<Import Project="$(MSBuildThisFileDirectory)..\Directory.Build.props" />
Copy link
Member

Choose a reason for hiding this comment

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

Maybe / like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

msbuild does a normalization, the relevant change was the casing. I think I will go back to backslash for everything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should add that as comment then?

I had a comment in the actual commit, but yes, will add a source comment

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

I'd love to see a bit less "WIP"-Dependencies but overall that's a good step forward.

The version numbers are arbitrary. If it makes you feel better I can push a 1.0 right now ;D

If you want, we can also change to a "non-branded" package that is owned by you guys or the fsfoundation.

"0x53A.ReferenceAssemblies.Paket" was just so that I could get started.

Similar, I forked dotnet-mergenupkg to dotnet-0x53A-mergenupkg so that I could work on this now, we can wait with actually pulling this until enrico has updated his original package to netcoreapp2.0.

@matthid
Copy link
Member

matthid commented Aug 26, 2017

Note that it's not a blocker for me :). I trust you to cleanup eventually. That's why approved the changes ;)

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

There wasn't any real need for this PR, I mostly wanted to test this out so I can also move my work solution to this model. So it is up to you whether we pull it now and clean up later, or wait a bit.

@forki
Copy link
Member

forki commented Aug 26, 2017

The version numbers are arbitrary

Welcome to netcore ecosystem ;-)

build.fsx Outdated
@@ -364,7 +364,7 @@ Target "MergeDotnetCoreIntoNuget" (fun _ ->

let runTool = runCmdIn "tools" dotnetExePath

runTool """mergenupkg --source "%s" --other "%s" --framework netstandard1.6 """ nupkg netcoreNupkg
runTool """0x53A-mergenupkg --source "%s" --other "%s" --framework netstandard1.6 """ nupkg netcoreNupkg
Copy link
Member

Choose a reason for hiding this comment

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

I think this fixes #2645. This is bad. We didn't even realize that we still depend on SDK1.0 to be installed. Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are welcome ;)

@0x53A
Copy link
Contributor Author

0x53A commented Aug 26, 2017

image

@0x53A 0x53A closed this Aug 26, 2017
@0x53A 0x53A reopened this Aug 26, 2017
0x53A added 2 commits August 26, 2017 20:33
* acquire msbuild via RoslynTools.MSBuild
* acquire reference assemblies via a self-built package based on https://github.com/jaredpar/xcopy-msbuild
* use a fork of dotnet-mergenupkg which was recompiled against netcoreapp2.0 (original was netcoreapp1.0) to remove dependency on 1.x runtime
@0x53A 0x53A changed the title [WIP] Make paket build on a clean windows [Ready] Make paket build on a clean windows Aug 27, 2017
@0x53A
Copy link
Contributor Author

0x53A commented Aug 27, 2017

Note: I would like to add an actual CI leg for this, but afaik AppVeyor does not provide "clean" windows machines, so I don't know of a way to test this.

@matthid
Copy link
Member

matthid commented Aug 27, 2017

Maybe with travis a minimalistic netcore only build?

@0x53A
Copy link
Contributor Author

0x53A commented Aug 27, 2017

Maybe with travis a minimalistic netcore only build?

On linux, I still rely on the system-installed msbuild because the one from RoslynTools.MSBuild crashes hard

@matthid
Copy link
Member

matthid commented Aug 27, 2017

"dotnet msbuild" could work. I mean there is a bit of work to do to make this happen but it's not too unrealistic as paket/fake are basically ported. I think the only thing I know is that the netcore version of paket cannot push for some reason

@0x53A
Copy link
Contributor Author

0x53A commented Aug 27, 2017

Officially, the dotnet cli does NOT support building old-sdk projects.

I would wait until we have ported paket completely to new-sdk.

@matthid
Copy link
Member

matthid commented Aug 27, 2017

then we need to update sooner or later anyway, because I really want to get rid of mono. We can still have tests and stuff working with mono but the core stuff will probably move soon :)

@0x53A
Copy link
Contributor Author

0x53A commented Aug 27, 2017

yes, it would be great to move paket itself to netcoreapp, maybe even standalone so the dotnet runtime does not need to be installed.

Do you know what the status of the linker is? Single-file standalone would be awesome.

@matthid
Copy link
Member

matthid commented Aug 28, 2017

I would be so happy already if stuff would be working, so I didn't think about a linker yet ;)

But yeah it would be awesome.

@enricosada
Copy link
Collaborator

@0x53A released dotnet-mergenupkg v2.0.0

This support both sdk 1.0 and 2.0 as cli tool

@forki forki merged commit 2d8be87 into fsprojects:master Sep 3, 2017
@forki
Copy link
Member

forki commented Sep 3, 2017

AWESOME!!!!

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