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

WIP: Dotnet new template #1989

Merged
merged 3 commits into from
Jun 10, 2018
Merged

Conversation

kblohm
Copy link
Contributor

@kblohm kblohm commented Jun 8, 2018

Hi,
i created a template to make it easier to get started using Fake.
I would appreciate some feedback if you are interested in this in general.
Some stuff to maybe discuss:

  • naming of parameters
  • available options
  • should this be in this repo at all?

Somewhat related:
What do u think about a Fake.All metapackage, similar to the AspNetCore.All? That would make it easier to get started. I personally do not really care about a few more megabytes to download to run my build. After the script is pretty much finished i can still clean up if i feel its getting too big. I am also kind of unsure what to include in a template like this, and a Fake.All package would get rid of that problem.

@matthid matthid changed the base branch from master to release/next June 9, 2018 13:15
@matthid
Copy link
Member

matthid commented Jun 9, 2018

Yes Metapackages is something that came up very early and yes it is not clear at all what Fake.All would include. This is even true if we want to bundle some areas like Fake.DotNet into a metapackage. Would we include NuGet AND Paket for example or only give a opinionated set?

I guess we would give some opinionated commonly used packages.

@matthid
Copy link
Member

matthid commented Jun 9, 2018

Regarding the change itself: Yes I'd like to see a template. Can we add some documentation around this? (in particular for me to see how this works ;) )

@kblohm
Copy link
Contributor Author

kblohm commented Jun 9, 2018

Would we include NuGet AND Paket for example or only give a opinionated set?

Do you mean the modules?
I would include all the modules inside a Fake.All package. Pretty much everything but the runtime. And if there is demand, create more dedicated metapackages, for example everything in Fake.DotNet.

* Update documentation in template.json
@kblohm
Copy link
Contributor Author

kblohm commented Jun 9, 2018

If you want to try it locally (and dont want to build stuff yourself) i can also upload the nupkg.

@matthid
Copy link
Member

matthid commented Jun 9, 2018

No worries. I'll probably just merge straight to staging :)
Just something about the scripts. Will they all be in the created "project"? Should we have some defaults or ask the user if he wants scripts at all (and if yes which one)? Is this even possible?
Also if we decide to include all should we name them fake.sh/fake.cmd to clarify that they are executing fake?

@matthid
Copy link
Member

matthid commented Jun 9, 2018

Also out of curiosity: Can you use the template on top of an existing project?

@kblohm
Copy link
Contributor Author

kblohm commented Jun 9, 2018

The build.fsx will always be created (you can specify the name though). Everything else is optional. I can also make the script optional.
I can rename build.sh and build.cmd to fake.*. I just used build because that is used in all the bootstrapping examples. But fake.* might make more sense.
And yes you can use it on top of an existing project. All the dotnet new stuff will just abort if it would have to override a file and i am pretty sure you can not merge stuff into existing files. So if you have a project with a paket.dependencies for example, you would have to usedotnet new fake --dependencies none so no new file is created (or dotnet new --force fake to override).
It does also not create any project or solution files (expect the build.proj, if that is the prefered way to bootstrap). That has the advantage that you can use other default templates and also use it for C#.
I could however create a second template that creates a solution and an example project and maybe also a build-script that does some actual stuff, like generating documentation. Then we would need to come up with more names for templates.

@matthid
Copy link
Member

matthid commented Jun 9, 2018

because that is used in all the bootstrapping examples.

I'd say this is for historic reasons, maybe we should change there as well?

Anyway, very nice idea thanks for looking into that. Should I merge as is? I think it looks quite usable already.
We can figure out the best defaults and namings along the way. There are probably quite some corner cases like maybe use the template to create the scripts (without overwriting build.fsx), but we don't have to support everything at start.

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

I renamed the files, should be OK now.
Thanks for the feedback.
Two more things:

  • In your opinion, if i want to make a template with more then just a basic build setup, should i contribute to ProjectScaffold or should i add a second one here?
  • If you have a unix system, would you mind testing the fake.sh file? Once for dotnet new fake --bootstrap project and once for dotnet new fake --bootstrap tool? I am not sure if they actually work, i could only test with git-bash.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

In your opinion, if i want to make a template with more then just a basic build setup, should i contribute to ProjectScaffold or should i add a second one here?

Uah, I believe the community is kind of split around that. My personal opinion is that is has been superseded by dotnet sdk templates. What's left is a centralized place to get started with community provided setup (Paket, Fake, Expecto, Unquote, FSharp.Formatting, everything put together ready to use). So what's missing is a ProjectScaffold-like template and a place for the documentation. (Which in the end could just be the current ProjectScaffold repository and homepage). fsprojects/ProjectScaffold#324

ProjectScaffold could also just be a script executing the various templates around the ecosystem:
Ie. it executes this template and some others to create various projects (testing, hello world, fake, paket, fsharp.formatting) and then writes a fake script to integrate everything. No idea if this is a good idea in practice :)

I guess everyone with a strong opinion there can have a huge impact, so feel free to just send PRs for what you feel is a good way forward (open issues if you feel it could be controversial)

If you have a unix system, would you mind testing the fake.sh file? Once for dotnet new fake --bootstrap project and once for dotnet new fake --bootstrap tool? I am not sure if they actually work, i could only test with git-bash.

As I'm on windows as well we can either add a integration test around that or I can try to use windows subsystem for windows :). Will merge on staging and take a look.

@matthid matthid merged commit 3f24a8f into fsprojects:release/next Jun 10, 2018
@kblohm kblohm deleted the fakeTemplate branch June 10, 2018 11:11
@baronfel
Copy link
Contributor

I can test the Unix version of things. Could you provide a little detail about what exactly I should try out?

@matthid
Copy link
Member

matthid commented Jun 10, 2018

@baronfel I'll report back to you when we have fixed staging but then it's basically:

dotnet new --nuget-source https://www.myget.org/F/fake-vsts/api/v3/index.json -i "fake-template::*"
dotnet new fake --bootstrap project
dotnet new fake --bootstrap tool

@@ -1099,6 +1099,17 @@ Target.create "_DotNetPackage" (fun _ ->
else c.Common
} |> dtntSmpl) "Fake.sln"

// pack template
let templateProj = appDir @@ "fake-template"
DotNet.pack (fun c ->
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we just add it to Fake.sln?

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

@matthid
This:

let netCoreProjs =
!! (appDir </> "*/*.fsproj")

is probably the problem. The template is just a .proj file. I did also not add the template to the Fake.sln. dotnet sln add did not like that.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

@kblohm thanks didn't noticed, will try to fix :)

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

Yes it would be nice to add it to the solution file. That would probably not fix the deployment though, as

Target.create "DotNetCorePushNuGet" (fun _ ->
    // dotnet pack
    netCoreProjs
    -- (appDir </> "Fake.netcore/*.fsproj")
    |> Seq.iter(fun proj ->
        let projName = Path.GetFileName(Path.GetDirectoryName proj)
        !! (sprintf "nuget/dotnetcore/%s.*.nupkg" projName)
        -- (sprintf "nuget/dotnetcore/%s.*.symbols.nupkg" projName)
        |> Seq.iter (nugetPush 4))
)

would still not push it to nuget.
I am not really sure why dotnet sln add does not want me to add a .proj file.
We could probably just change it to fsproj.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

dotnet sln add did not like that.

Ok understood

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

I just had a look at https://github.com/aspnet/templating/tree/dev/src/Microsoft.DotNet.Web.ProjectTemplates. They are also just using .csproj files for their template projects. So the easiest way might be to just change it to fsproj and add it to the sln file. Then we could also remove the special handling.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

haha, probably for the very same reason :) Will try

@matthid
Copy link
Member

matthid commented Jun 10, 2018

@baronfel Please test with dotnet new --nuget-source https://www.myget.org/F/fake-vsts/api/v3/index.json -i "fake-template::5.1.0-alpha*"

@baronfel
Copy link
Contributor

as I expected you need to set the executable flag on the .sh script. This can be done with post-install actions, MiniScaffold does it like this: https://github.com/TheAngryByrd/MiniScaffold/blob/master/Content/.template.config/template.json#L19

@baronfel
Copy link
Contributor

The unique ids for the postactions that can be run is here: https://github.com/dotnet/templating/wiki/Post-Action-Registry, but I think you'll just need that exact code block from miniscaffold.

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

It should mainly be dotnet new fake --bootstrap tool. Because in fsprojects/FSharp.Control.Reactive#110 i did not manage to get this to run on travis and MAYBE its because of build.sh.
I am also not sure if the script can be run twice, because you have to check if FAKE is already installed, otherwise dotnet tool install will be angry.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

Now after catching this I think we need an integration test which ensures that dotnet new -i && dotnet new fake && ./fake.sh build works without error

@baronfel
Copy link
Contributor

is there a way to make the restore in fake.sh a one-time thing? it introduces a noticable amount of latency in using the script:

➜  fake-project git:(master) ✗ ./fake.sh
  Restore completed in 28.85 ms for /Users/chethusk/oss/fake-project/build.proj.
  Restore completed in 50.92 ms for /Users/chethusk/oss/fake-project/build.proj.

real	0m5.334s
user	0m4.109s
sys	0m0.897s

@baronfel
Copy link
Contributor

@matthid you can again look at MiniScaffold for how to set up such a test easily. The test suite there installs the new packed template locally and tests it.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

@baronfel This really is a .net sdk issue because it should do an implicit restore (I believe there is an open issue for that). Once that is fixed we can remove the explicit restore and let the sdk handle that.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

@baronfel Yes my thinking was that dotnet new --nuget-source /full/path/to/dir/containing/nupkg -i "fake-template::<version>" would work

In any case can you or @kblohm send a PR to add the test?

@baronfel
Copy link
Contributor

also if the user specifies a non-default script name I think the generated sh/cmd scripts should specify the provided name in the invocations, otherwise the new-ly created script name will not be run.

This is made harder by the fact that run and build have different formats for where the script file goes, so you'd almost have to do parsing of the first argument to see if it is build or run and change things based on that.

@baronfel
Copy link
Contributor

sorry for lots of comments, just noting things as I see them

@baronfel
Copy link
Contributor

installing a global tool to a subdir doesn't seem to work:

➜  fake-tool git:(master) ✗ ./fake.sh
You can invoke the tool using the following command: fake
Tool 'fake-cli' (version '5.0.0') was successfully installed.
The application to execute does not exist: '/Users/chethusk/oss/fake-tool/.fake/..//Users/chethusk/oss/fake-tool/.fake/.store/fake-cli/5.0.0/fake-cli/5.0.0/tools/netcoreapp2.1/any/fake-cli.dll'.

The binary gets placed in .fake/fake, but that binary has a path hard-coded into it that is not correct:

/Users/chethusk/oss/fake-tool/.fake/..//Users/chethusk/oss/fake-tool/.fake/.store/fake-cli/5.0.0/fake-cli/5.0.0/tools/netcoreapp2.1/any/fake-cli.dll

Note that the partially-mangled correct path is findable there: /Users/chethusk/oss/fake-tool/.fake/.store/fake-cli/5.0.0/fake-cli/5.0.0/tools/netcoreapp2.1/any/fake-cli.dll

@matthid
Copy link
Member

matthid commented Jun 10, 2018

also if the user specifies a non-default script name I think the generated sh/cmd scripts should specify the provided name in the invocations, otherwise the new-ly created script name will not be run.

Currently the scripts will just wrap fake and not actually build a target (which I think makes sense).

This is made harder by the fact that run and build have different formats for where the script file goes,

My opinion on this is that if people use a non-default script-name fake build will still work (as long as there is only ONE script). On the other side if people use something else than build.fsx they probably should know what they are doing

sorry for lots of comments, just noting things as I see them

No worries there, perfectly reasonable ;)

@baronfel
Copy link
Contributor

There's also a bug with install when dotnet cli is in a non-standard location: https://github.com/dotnet/cli/issues/9114

@baronfel
Copy link
Contributor

@matthid working on a PR now for chmod on the scripts + a test run

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

@baronfel thanks for looking into this. All that fancy linux stuff and not being able to test locally makes this kinda hard.

@baronfel
Copy link
Contributor

happy to help :)

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

So until dotnet/cli#9114 is fixed, you have to use the build.proj approach to run on unix, right? Or am i missing something in that issue?

@baronfel
Copy link
Contributor

well, you can also use the global tool, which is what I plan on standardizing on. should we make that a template-option? --bootstrap global-tool? and then skip the tool installation in the shell/cmd scripts?

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

That is what --boostrap none does.
Or do you mean not generate anything AND install the tool globally?

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

@baronfel

as I expected you need to set the executable flag on the .sh script.

So i can not create this on windows for you to run? That pretty much means whoever does the bootstrapping has to be on unix? What happens otherwise?

@matthid
Copy link
Member

matthid commented Jun 10, 2018

That pretty much means whoever does the bootstrapping has to be on unix? What happens otherwise?

Uah interesting point. On windows we would need to run git update-index --chmod=+x fake.sh but we don't even know if git is available and if it is if the user created a git repository already...

@baronfel
Copy link
Contributor

Yes but this is a problem we would have had even with the change-permissions activity. The BCL File Metadata structures didn't until recently have the capacity to even report file permissions, so it's taking even longer for that ability to trickle down into all the tools. It's very irritating.

@kblohm
Copy link
Contributor Author

kblohm commented Jun 10, 2018

Ahh! Thanks @matthid that helped! So yeah i can speak from experience now, that is a problem when you create the files on windows and try to build on unix.

@matthid
Copy link
Member

matthid commented Jun 10, 2018

Not sure if we can somehow build that into the template (it would be nice if we find a way). But if not it is not a huge deal chmod errors are quite common so I hope users are able to figure it out.

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