-
Notifications
You must be signed in to change notification settings - Fork 525
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
Dotnetcore build for Paket.Core #1785
Conversation
@matthid this is awesome! would you mind posting here simple steps (from scratch, assuming I don't have dotnet.exe installed) to try it out? I'd like to do some guinea pig testing on this, and could contribute to the documentation while you work on the heavy implementation stuff. |
@smoothdeveloper Sure, but please keep in mind that I'm currently only interested in the
You currently cannot run it. To run it one needs to convert the Cli project as well (which I don't need for now). A working Argu package can be found in my Fake branch (fsprojects/FAKE#1281) ... Besides that I think no further dependencies are needed. Conversation would be simply copying the project.json from |
@matthid I'm working on refactorings in |
@forki I think the netstandard stuff was broken, could you review 1df0579 ? Can we somehow specify that a portable package is only compatible in the other way? IE I think they should not (at least not by default) be added when compiling a NetStandard library. On the other hand when compiling for Portable you can add portable profiles. I tried this But it seems like its the wrong direction :( |
I made the following changes:
A careful review would be appreciated. I added a couple of tests for the new behavior. |
@@ -400,7 +438,7 @@ type InstallModel with | |||
|
|||
member this.AddTargetsFiles targetsFiles = InstallModel.addTargetsFiles targetsFiles this | |||
|
|||
member this.AddPackageFile (path, file, references) = InstallModel.addPackageFile path file references this | |||
//member this.AddPackageFile (path, file, references) = InstallModel.addPackageFile path file references this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking this was a public API, but it wasn't used anywhere and I'd like to get rid of it because it doesn't consider the new
"ref" folder.
I have no idea what AppVeyor and Travis are trying to tell me, locally all tests are working. Any idea what might be wrong? I cannot reproduce :( |
I managed to write a failing unit test (which of course is not failing on my local machine)... |
Oh my god. Problem is 15256bd which is not contained in my changes jet. But AppVeyor and Travis are building the merged state :(. This took me quite some time to realize... |
Yeah everything is green again. |
I cherry picked from this like crazy and tried to get everything in that is unrelated to .NET Core. Can you please rebase and take a look? |
you should cherry pick 1650f39 as well . I try to take a closer look later... |
I think I already picked 1650f39 by accident. |
Wow I'm trying to get this back to a mergeable state. Next time we should find a better workflow. Just force me to send smaller PRs (even if its not ideal for me as I'm finding all these things while trying to focus on one huge thing). Locking over the commits I see that some have been applied only partially so I might miss some things now. Or we could have just merged the whole thing (all tests were green anyway, just the testing of the dotnetcore stuff wasn't there). I think suggesting cherry picking wasn't a good suggestion on my part after all... Anyway congrats that you managed to make a release of all of this and extracting the hidden pieces :). |
0e2315c
to
d489014
Compare
Hm maybe it wasn't as bad as expected :) |
chessie 0.6 should work on coreclr |
@forki Thanks, using chessie 0.6 now and removed the old binaries |
we need a nuge.config that clears all inherited crap, right? |
Doesn't exactly look like its working though.
I have no idea. @enricosada says no nuget.config is needed, correct? |
you always want to delete the crappy machine wide config ;-) |
I'm not against it but didn't I just delete one after the review? |
I just added it and now things work |
Ok good to know :) So we should always have one basically. |
that whole concept is nuts, but yes |
No idea on CI and on my machine everything seems to work... |
ok will investigate tomorrow |
no Sry for be late, anyway @forki as a note, to use dotnet sdk you can also use binaries (zip) instead of installer (msi). In every os. a dotnet core sdk install is just
like that it you doesnt mess with your machine (it's just a directory) and you can also use multiple version in parallel The build script does just that (download+unzip). binaries are in the too much hidden |
no we need the tag. othwerwise the package feeds from machine settings are used. which are wrong on my machine |
btw: it's merged and released! Thanks for this amazing work |
ah ok, better! |
Something is wrong: https://www.nuget.org/packages/Paket.Core Paket.Core should not depend on FSharp.Core alpha for normal .NET |
is not depending FSharp.Core for normal dotnet <dependencies>
<group targetFramework=".NETStandard1.6">
<dependency id="System.Xml.XmlDocument" version="[4.0.1, )" />
<dependency id="System.Net.Requests" version="[4.0.11, )" />
<dependency id="System.Diagnostics.TraceSource" version="[4.0.0, )" />
<dependency id="System.Xml.XPath.XmlDocument" version="[4.0.1, )" />
<dependency id="System.Diagnostics.Process" version="[4.1.0, )" />
<dependency id="System.Xml.XDocument" version="[4.0.11, )" />
<dependency id="System.Xml.XPath.XDocument" version="[4.0.1, )" />
<dependency id="System.Security.Cryptography.ProtectedData" version="[4.0.0, )" />
<dependency id="System.Security.Cryptography.Algorithms" version="[4.2.0, )" />
<dependency id="System.Diagnostics.FileVersionInfo" version="[4.0.0, )" />
<dependency id="NETStandard.Library" version="[1.6.0, )" />
<dependency id="Newtonsoft.Json" version="[9.0.1, )" />
<dependency id="FSharp.Core" version="[4.0.1.7-alpha, )" />
<dependency id="Mono.Cecil" version="[0.10.0-beta1-v2, )" />
<dependency id="Chessie" version="[0.6.0, )" />
</group>
</dependencies> it does depende on |
which would be wrong as well. |
let me try locally why merge doesnt work as expected. it shoudl create a fallback |
ok trying.
i have
and it's using |
@forki built locally is ok.
<dependencies>
<group>
<dependency id="Chessie"></dependency>
<dependency id="FSharp.Core"></dependency>
<dependency id="Mono.Cecil"></dependency>
<dependency id="Newtonsoft.Json"></dependency>
</group>
<group targetFramework=".NETStandard1.6">
<dependency id="System.Xml.XmlDocument" version="[4.0.1, )"></dependency>
<dependency id="System.Net.Requests" version="[4.0.11, )"></dependency>
<dependency id="System.Diagnostics.TraceSource" version="[4.0.0, )"></dependency>
<dependency id="System.Xml.XPath.XmlDocument" version="[4.0.1, )"></dependency>
<dependency id="System.Diagnostics.Process" version="[4.1.0, )"></dependency>
<dependency id="System.Xml.XDocument" version="[4.0.11, )"></dependency>
<dependency id="System.Xml.XPath.XDocument" version="[4.0.1, )"></dependency>
<dependency id="System.Security.Cryptography.ProtectedData" version="[4.0.0, )"></dependency>
<dependency id="System.Security.Cryptography.Algorithms" version="[4.2.0, )"></dependency>
<dependency id="System.Diagnostics.FileVersionInfo" version="[4.0.0, )"></dependency>
<dependency id="NETStandard.Library" version="[1.6.0, )"></dependency>
<dependency id="Newtonsoft.Json" version="[9.0.1, )"></dependency>
<dependency id="FSharp.Core" version="[4.0.1.7-alpha, )"></dependency>
<dependency id="Mono.Cecil" version="[0.10.0-beta1-v2, )"></dependency>
<dependency id="Chessie" version="[0.6.0, )"></dependency>
</group>
</dependencies> |
@enricosada not sure if we should always use path or always install our own version. I tend to always install... |
@forki yes the version you uploaded seems to be the netstandard version only? maybe temp/dotnet got uploaded first? no idea what paket push is doing there. |
@matthid about dotnet location, we should use the dotnet in PATH. i think the flow should be install steps..
And normally
fake should always expect that dotnet exists and is in |
@matthid yes it's the wrong package (the netcore), probably the publish is sending the wrong package |
this is weird. this was green, but the mono build is still red after merge because of issue with dotnet core installation |
let me fix that, is easy now instal netcore, ref #1914 |
TODO (this PR):
dotnet build -f net46
dotnet build -f netstandard1.6
TODOs -> Later, see #1909