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

fix packaging of FSharp.Core and Paket/Paket.Bootstrapper as tool #3734

Merged
merged 37 commits into from
Nov 28, 2019

Conversation

enricosada
Copy link
Collaborator

@enricosada enricosada commented Nov 26, 2019

fixes for packaging and build script

build script

  • publish exe in bin/net461 and bin/netcoreapp2.1. Do not use bin_netcore anymore.
  • pack nupkg in temp
    • for Paket.Core, is done by paket pack only
    • for Paket/Paket.Boostrapper is done by dotnet pack, because are .net tools
  • .NET Core Sdk is assumed to exists always (the sdk is always installed because
    needed for build), so that simplified a bit the build script
  • use dotnet test to run tests, remove nunit-console

Others

  • run tests in netcoreapp3.0 not netcoreapp2.1, because netcoreapp3.0 is bundled with sdk
  • run paket boostrapper unit tests too in the RunTests target
  • add SkipTests build script parameter to skip all tests (both unit and integration)
  • in travis use xenial with .net core 3.x preinstalled
  • workaround for dotnet test hang on travis (ref Tests hang from dotnet test microsoft/vstest#2080 )
  • add boostrapper to appveyor artifacts

remove dotnet-mergenupkg

when Paket and Paket.Boostrapper are donet pack'd with PackAsTool=true property (to pack as .NET Tool) these will include the merged paket.exe/paket.boostrapper.exe inside the package in the tools dir.

The PackAsTool=true will also force to target netcoreapp only, because net461 is not supported.
In development these projects (Paket/Paket.Boostrapper) continue to target both net461 and netcoreapp2.1

no need to dotnet mergenupkg the Paket.Core package, because now it multitarget
correctly (it's only one package)

fix duplicated netstandard tfm in Paket.Core

Paket.Core

set min version of dependencies, so are set on package deps set min version constraints for dependencies, so are set in the nuspec generated by paket pack for FSharp.Core
Otherwise, dotnet restore of FSharp.Core use really old version of Newtonsoft/etc who may not work on .NET Standard.
For example newtonsoft was restored as v6.x who support only net461 (valid for netstandard, but generate warnings and fails on .net core)

  • bump mono.cecil to stable 0.11 (who support netstandard2.0)

Paket as tool

run as dotnet paket

  • workaround for transitive of FSharp.Core not pulled when packaging as PackAsTool
  • copy pdbs of package references into Paket tools package allow to get meaningful stacktraces also for deps (a bit bigger size but is worth it)
  • remove binding redirects from netcoreapp, leave it only in net461
  • no need for xml inside tool nupkg
  • add package metadata, because tool use dotnet pack not paket pack
  • add release notes to the package, who is created with dotnet pack no paket pack`

Paket.Bootstrapper as tool

run as dotnet paketbootstrapper

  • dotnet tool doesnt allow command with dot (.), use paketbootstrapper instead as command name (exe name is the same)
  • copy pdbs of package references into Paket tools package allow to get meaningful stacktraces also for deps (a bit bigger size but is worth it)
  • add package metadata, because tool use dotnet pack not paket pack
  • add release notes to the package, who is created with dotnet pack no paket pack`

Enrico Sada added 3 commits November 26, 2019 17:29
- publish exe in `bin/net461` and `bin/netcoreapp2.1`
- pack nupkg in `temp`
- net core is assumed to exists always (the sdk is always installed because needed for build)

## remove dotnet-mergenupkg

when `Paket` and `Paket.Boostrapper` are `donet pack` with `PackAsTool=true` property (to pack as .NET Tool)
these will include the merged paket.exe/paket.boostrapper.exe into the `tools` dir

no need to dotnet mergenupkg the Paket.Core, because now multitarget correctly (it's only one package)
@enricosada
Copy link
Collaborator Author

@forki i'll need to check the full run of integration tests in ci to see if ok

@@ -2,11 +2,5 @@
"version": 1,
"isRoot": true,
"tools": {
"dotnet-mergenupkg": {
Copy link
Member

Choose a reason for hiding this comment

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

not needed any more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, is not not needed anymore.
before with dotnet mergenupkg i added the .NET tools (from the netcoreapp package) inside the package with tools/paket.exe.
now it does the opposite, it create the .NET tools package (netcoreapp) with dotnet pack and when packaging add also the already built tools/paket.exe.

Now is easier to do with .net sdk but it also needed some tweaks to build script that i added in this PR, before was just easier dotnet mergenupkg , to make it the build script less complicated

@enricosada
Copy link
Collaborator Author

This PR is already big enough.
I'll leave MS sourcelink for next PR.
i checked the pdb with sourcelink test and seems ok

@enricosada
Copy link
Collaborator Author

i tried to add a smoke test to dotnet tool install the package, but i failed to do it safely (without cache).

the --no-cache doesnt work (because c:\users\%USERNAME%\.nuget\packages is a source, not a cache, go figure, ref NuGet/Home#5619 )

i tried use a nuget.config like

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <config>
    <add key="repositoryPath" value="C:\temp3\clientpr\packages\" />
    <add key="globalPackagesFolder" value="C:\temp3\clientpr\packages\" />
  </config>
  <packageSources>
    <!--To inherit the global NuGet package sources remove the <clear/> line below -->
    <clear />
    <!-- <add key="nuget" value="https://api.nuget.org/v3/index.json" /> -->
    <add key="pr" value="c:\temp2\feedpr\feed" />
  </packageSources>
</configuration>

the globalPackagesFolder setting does the trick and the package is saved in that dir instead of the global nuget cache/source, running

dotnet tool install paket.bootstrapper --version 5.238.1 --no-cache --configfile nuget.config -v n

is also download, added to .config and success message.

You can invoke the tool from this directory using the following commands: 'dotnet tool run paket' or 'dotnet paket'.
Tool 'paket' (version '5.238.1') was successfully installed. Entry is added to the manifest file C:\temp3\clientpr\.config\dotnet-tools.json.

But doesnt work

C:\temp3\clientpr>dotnet paket
Run "dotnet tool restore" to make the "paket" command available.

because search in global nuget cachen and no option in dotnet tool run to specify another.

le sigh

@forki
Copy link
Member

forki commented Nov 27, 2019

green. ready to merge?

@enricosada
Copy link
Collaborator Author

enricosada commented Nov 27, 2019

Really near but not.
I don’t see mono.cecil is the tools directory if the Paket package after dotnet pack /p:PackAsTool=true.
Trying to understand why

@forki
Copy link
Member

forki commented Nov 27, 2019

is it ILMerged?

@enricosada
Copy link
Collaborator Author

enricosada commented Nov 28, 2019

is it ILMerged?

@forki no, the issue is in the netcoreapp2.1 published as FDD, who is not ilmerged.
ilmerged is only net461

INFO: IL Repack - Version 2.0.18
INFO: ------------- IL Repack Arguments -------------
/out:bin\merged\paket.exe  bin\net461\paket.exe bin\net461\Paket.Core.dll bin\net461\FSharp.Core.dll bin\net461\Newtonsoft.Json.dll bin\net461\Argu.dll bin\net461\Chessie.dll bin\net461\Mono.Cecil.dll
-----------------------------------------------

I expect the p2p dlls inside the publish dir if i do dotnet publish src/Paket -f netcoreapp2.1.
Dunno if is a bug, meanwhile i workaround, i'll check later to minimize the repro.

Let me rerun some tests locally before merge, but afaik should be ok if green, no more known issues

Enrico Sada added 2 commits November 28, 2019 15:21
…er` instead

cannot use `dotnet paket.bootstrapper` because of the `.` (tool restore correctly, but fails to run)
using `dotnet paketbootstrapper` instead
set min version constraints for dependencies, so are set in the nuspec generated by `paket pack` for `FSharp.Core`
Otherwise, `dotnet restore` of `FSharp.Core` use really old version of Newtonsoft/etc who may not work on .NET Standard.
For example newtonsoft was restored as v6.x who support only net461 (valid for netstandard, but generate warnings)
@enricosada
Copy link
Collaborator Author

enricosada commented Nov 28, 2019

i rechecked locally:

  • Paket.Core in a net461/netcoreapp2.1 project
  • Paket as local tool (dotnet paket)
  • Paket.Boostrapper as local tool (dotnet paketboostrapper renamed to make it work), but ihmo is useless or worse

all ok afaik.

There are misc fixes/enhancement documented in the PR description, but should all be ok

@forki so if green, merge it! :shipit:

@enricosada enricosada changed the title fix build script fix packaging of FSharp.Core and Paket/Paket.Bootstrapper as tool Nov 28, 2019
@forki
Copy link
Member

forki commented Nov 28, 2019 via email

@enricosada
Copy link
Collaborator Author

Can we use bootstrapper as global tool? In magic mode?

@forki no, i just packaged as tool. because dunno what you want to do.

But ihmo is not the way forward with that.

I think about that scenario a lot for repotool/.net core paket.

Scenario is a nice dev ux + deterministic builds

I think can be done in two with:

a paket global tool who can do:

  • run init like command
  • all other commands, it will execute the dotnet paket (so will run the local paket, deterministic/pinned) passing the same args who got (like magic mode)
  • seem like bootstrapper in magic mode, but is 2% of bootsrapper code

a paket local tool who can do:

  • pinned, so deterministic build
  • all usual commands

Using the bootstrapper ihmo is not needed, because:

  • just 2% of bootstrapper code is needed
  • use a single package -> better experience (because naming)

What change without the boostrapper, list of features:

  • pinning. OK is done by tool manifest
  • specify a different feed. OK with nuget.config or dotnet tool restore args
  • download from nuget package. OK the default
  • download from github releases. KO not supported. BUT with global nuget cache is okish because cached = fast
  • caching of version. OK in the global nuget cache
  • magic mode machinery/optimization. OK as local+global tool, have the same name. see below for info
  • self updates. OK, done with dotnet tool update

So my idea is:

let's use just paket tool. will be paket (global) and dotnet paket (local)

In the first lines of paket main, before argu parse, we include a check.

  • if is init command -> then do it. OK because we know first word
    • on init:
      • generate paket.dependencies as usual
      • check if exists tool manifests, otherwise generate it
      • run dotnet tool install paket --local
  • else
    • recursive search directory hierarchy for .paket dir.
    • if exists, it's a paket repo. good
    • if exits tool manifest (.config/dotnet-tools.json):
      • then run dotnet paket with args
      • else run ./paket/paket.exe with args (backward compatibility, no more need for alias)

PRO:

  • developers use global tool or local tool
  • CI/Docker use ONLY local tool, no need for global tool

The only unknown i know:

  1. clean repo scenario

given a clean repo (git clone/git clean -xdf), i dont know exactly how too install paket if i do just dotnet build.
If is already restore the tools (dotnet tool restore, like in CI), works. Otherwise dunno (because parallel msbuild etc)
But is okish because we can do dotnet tool restore in the build.cmd (it's fast noop if already restore)

More tied to dotnet cli args (if they change we may need tweaks), but most of the corner cases are inside the target file anyway

I'll try send a PR, should not be lot of work to show it

@forki
Copy link
Member

forki commented Nov 28, 2019 via email

@enricosada
Copy link
Collaborator Author

@forki i know, it was the same idea time of months ago, just not implemented until local tool.
I'll give it a shot

BTW @forki diffing things i seen two bugs in this PR (but merge it anyway)

  • I dont see the icon either (i can try fix that later)
  • the generate exe is not digitally signed correctly anymore (i think). I dont really know that signing stuff well (antivirus whitelist?) just saying if someone can help with it

i tried diff the generated exe and old versions and seems ok

@enricosada
Copy link
Collaborator Author

@forki there are some flaky tests on travis, but one of two builds was green (i rerun the failed one, now is green, i think can be set FLAKY later)

@forki
Copy link
Member

forki commented Nov 28, 2019 via email

@enricosada
Copy link
Collaborator Author

enricosada commented Nov 28, 2019

Good!

about the icon:

  • was added in 8ee2bf9
  • the <Win32Resource>paket.res</Win32Resource> in fsproj doesnt seem to work
  • in csproj should be <ApplicationIcon>paket.ico</ApplicationIcon> instead, more easy, but we are fsproj :D

I'll check in another PR

@forki
Copy link
Member

forki commented Nov 28, 2019 via email

@enricosada
Copy link
Collaborator Author

So, for icon bad luck atm.

in F# we need to use native resource like <Win32Resource>paket.res</Win32Resource>, not ApplicationIcon ( ref dotnet/sdk#2818 ).
But is not yet implemented in clr fsc compiler (ref dotnet/fsharp#1172 )

i'll try a tool to fix it after generation, but in another PR, this is done @forki

@forki forki merged commit a1b7ad3 into fsprojects:master Nov 28, 2019
@forki
Copy link
Member

forki commented Nov 28, 2019

Awesome. Thanks for the work

@Krzysztof-Cieslak
Copy link
Member

a paket global tool who can do:
...
seem like bootstrapper in magic mode, but is 2% of bootsrapper code

So 2 questions about that:

  1. Will paket global tool understand version pinning from paket.dependencies file?
  2. Will paket global tool download latest version of Paket if version is not pinned?

@enricosada
Copy link
Collaborator Author

enricosada commented Nov 29, 2019

Will paket global tool understand version pinning from paket.dependencies file?

@Krzysztof-Cieslak if you mean version in paket.dependencies, depends (same for paket.exe.config, i use that more).

  • for current .paket/paket.exe, yes, because paket global tool will run .paket/paket.exe so that (real or magicmode) will read version and use pinned version
  • for dotnet paket (paket as local tool), no, BUT will be pinned in .config/dotnet-tool.json (the tool manifest of local tools)
    for for both, will use the right pinned version afaik

Will paket global tool download latest version of Paket if version is not pinned?
we can discuss about behaviour (i think is a bad idea to not pin, because can break if .paket/paket.restore.targets is not aligned) and preserve old behaviour while forcing pinning for new one.

  • for dotnet paket (paket as local tool), is pinned in the manifest. We can print a message maybe if is not the latest version (like yarn does afaik) so user can update if they want.
  • for .paket/paket.exe if is magic mode, same behaviour. it's downloaded first time (respecting paket.exe.config and paket.dependencies version, after that usual check

but we can also just skip paket global tool and not support older .paket/paket.exe .

I'll open a PR with this

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