Implement a loading script generator. #1619

matthid · 2016-04-18T13:08:10Z

See #1613. This is a new PR against the v3 branch.

As I don't know when I can continue this, here is a start for
#1593
#677

instead of #702 we resolve everything our-self. I don't know when I can finish this so here is what I have for now.

I think it works for Deedle for now...

Todo:

Handle dependencies within the package
Handle Framework dependencies?
Test the code
Add Mono.Cecil to ILMerge list
Integrate in Paket or a separate project
Rebase and remove "fix failing test case" commit -> Fix failing test case. #1644
Create documentation for this
Create Issue/PR/Feature-Request for top level include scripts (for a group)
Create Issue/PR/Feature-Request for generating include scripts on restore

matthid · 2016-04-18T13:09:56Z

@smoothdeveloper I rebased against the latest v3 branch and created a new generate_scripts_v3 branch.

matthid · 2016-04-18T17:43:05Z

Ok I did another round of changes. I think this is now more or less feature complete (at least how I imagined it).
As a small bonus I added a script generator for C# scripts as well.

One minor thing are probably framework assemblies, but when this hits someone it shouldn't be too difficult to add.

@forki how should we integrate this feature?
Into paket.core and generating scripts for all defined frameworks on 'restore' (= all if user has no framework restrictions)? As a separate command? On top of paket as a separate tool (executed via FAKE)?

@smoothdeveloper Feel free to submit further changes and thank you for the help. Sorry if you have some open changes, as now you probably have a lot of merge conflicts ...

smoothdeveloper · 2016-04-18T17:49:07Z

src/Paket.LoadingScripts/LoadingScriptsGenerator.fs

+    let depLines =
+      input.DependentScripts
+      |> Seq.map (fun s -> sprintf "#load \"../%s\"" (s.Replace("\\", "/")))


I think we need a helper function given a (assembly: FileInfo) and a (packageFolder: DirectoryInfo) generate a system agnostic path.

those s.Replace here and there seems hackish.

Yeah and we need to escape the string properly as well. This whole thing is a bit hacky atm :)

I'll give it a spin, trying to not expand too much the code (you did a good slimming of things which ended not being useful from my changes).

smoothdeveloper · 2016-04-18T18:03:50Z

@matthid: thanks for pushing :)

Seems you got rid of QuickGraph and figured out a way with just Mono.Cecil, sounds great.

I'm not sure we want scripts generated on paket restore, seems nice but not all users want include scripts (do they?).

I'm also not sure about the location and naming of scripts, the code should also be more resilient to such adjustments, also maybe it is better to give end user option for alternative location for the scripts to be generated?.

@forki: is it ok to add Mono.Cecil (274kb)?

smoothdeveloper · 2016-04-18T18:11:08Z

@matthid, we might need to deal with native binaries also, one example I found is SkiaSharp nuget package has native dll file under build/x86 and build/x64, in F# either of those directories should be included with #I directive.

I'm not sure yet if paket handle those files properly in terms of putting such a dependency in paket.references but I assume it does.

matthid · 2016-04-18T18:50:33Z

we might need to deal with native binaries

I have no idea. But it is likely that (if paket can handle those binaries) we can re-use something from the InstallModel class to get the required infos. If not I would say it's a paket limitation and nothing we need to figure out as part of this PR?

smoothdeveloper · 2016-04-19T14:42:31Z

I'm getting on top of changes right now, will update the PR shortly today.

smoothdeveloper · 2016-04-19T16:04:41Z

@matthid, I've made a bunch of changes, please check it out, I think it looks great.

I've tried it out on few repositories using paket and it seems to be doing great for a first take at this 😄

smoothdeveloper · 2016-04-19T21:14:08Z

smoothdeveloper · 2016-04-19T21:36:03Z

@matthid I think we need to to work on those points:

ponder on the case where no assembly nor dependent script is going to land in the script (see include.fsharp.core.fsx)
figure out the exception when we run the code on Paket working copy itself and find a way to recover from it to generate what can be generated
handle the error cases because packages, files are not restored, existing

@forki how do you forsee integrating this into the main tool (we need only mono.cecil now)

forki · 2016-04-19T21:39:11Z

Currently I'm focused on other stuff but I don't mind to depend on Cecil.
(tbh I had another use case in mind that would also depend on Cecil, but no
time for that right now)
On Apr 19, 2016 11:36 PM, "Gauthier Segay" notifications@github.com wrote:

@matthid https://github.com/matthid I think we need to to work on those
points:

ponder on the case where no assembly nor dependent script is going
to land in the script (see include.fsharp.core.fsx)

figure out the exception when we run the code on Paket working copy
itself and find a way to recover from it to generate what can be generated

handle the error cases because packages, files are not restored,
existing

@forki https://github.com/forki how do you forsee integrating this into
the main tool (we need only mono.cecil now)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1619 (comment)

matthid · 2016-04-19T21:41:03Z

src/Paket.LoadingScripts/LoadingScriptsGenerator.fs

+
+          // note: we should probably find/use Paket way to resolve package group folder
+          match groupName.GetCompareString () with
+          | "main"    -> packagesFolder


should we replace this with the constant as well?

matthid · 2016-04-19T21:49:34Z

the case where no assembly nor dependent script is going to land in the script (see include.fsharp.core.fsx)

I see this more like cosmetics, can be fixed later if people have problems with it. Removing would complicate some things at this point.

errors / exception

If we cannot create one script we cannot really create any scripts that depend on it... or we just keep going.

We should just let it fail and write the raw problem to the output (and continue whatever paket was doing). People will complain and we can fix issues as they pop up.

Or did you mean something else?

matthid · 2016-04-19T21:51:33Z

By the way a good set of changes, thanks. You really like the System.IO classes ;) I somehow never find them usable, but thats probably just me :)

smoothdeveloper · 2016-04-19T22:09:51Z

@forki

I don't mind to depend on Cecil.

Awesome 😄

@matthid

It doesn't feel 'right' that the generator has to call the installmodel. Instead this type should contain all the results (for example the list of references instead of the installmodel). What do you think?

yes the overall structure didn't feel natural before I did last change, and I don't think I improved it much, we should work on making the code feel nice and simple.

Maybe we also need some integration testing, not clear yet but I'll give a look at how we can do that in current set of integration tests which have great infrastructure.

I see this more like cosmetics, can be fixed later if people have problems with it. Removing would complicate some things at this point.

Good point

If we cannot create one script we cannot really create any scripts that depend on it... or we just keep going.

We should generate other scripts in other branches of the dependency tree, not sure how easy that is but sounds achievable and useful.

We should just let it fail and write the raw problem to the output (and continue whatever paket was doing). People will complain and we can fix issues as they pop up.

Yes we need to see what happens from user perspective.

Also, what do we intend end user to do?

You really like the System.IO classes

They just make things a bit more obvious, otherwise I always wonder is this a folder, a file, etc. They incur a bit of noise (.FullName) but also reduce it (get name, get parent, etc.), better guarantees IMHO.

smoothdeveloper · 2016-04-21T18:43:07Z

@matthid, I've restored better integration points to pass higher order function for:

custom script generators
script file location resolution.

I'm thinking of tackling writing some crude test, and in order to do so, leverage some of the existing integration test cases.

I'll add a ScriptGenerationTests module in the integration tests project and see how it goes.

baronfel · 2016-04-21T18:56:56Z

Some points came up on slack:

It would be great to have a paket.dependencies flag to enable script generation. something like generate_scripts: on that could be placed at a global, group, or individual package level, like all the other options.

Also, has there been any consideration to restoring/generating the scripts on paket restore? The use case I'm thinking of is a build server/CI/FAKE script flow where

ci Server clones repo
ci server calls build(.bat/.ps1/sh)
ci server invokes FAKE with script
FAKE script #load script includes.

This isn't a deal-breaker, because it's easy enough to have the bat/ps1/s script call the generate-scripts functionality, but it may be nice.

baronfel · 2016-04-21T19:13:32Z

My biggest concern with the generate_scripts in the dependencies file is that implies the options state being added to the lockfile, and I'm not sure that we're in the habit of adding non-dependencies-related information to the lockfile. I could be persuaded to drop that whole line of investigation 😃

matthid · 2016-04-21T19:18:30Z

@smoothdeveloper Yes that all sounds great. I might be able to take a closer look, handle framework dependencies and write some tests on the weekend :)

@baronfel good idea with the paket.dependencies configuration. And I agree if we include this in paket we should probably generate the scripts on restore.

baronfel · 2016-04-21T19:24:32Z

@matthid I've done work in the dependencies and lock file parsers before. If you decide to go that route I'd be glad to tackle that if you're busy doing other things.

matthid · 2016-04-21T19:28:41Z

@baronfel good point. well we didn't do any integration at this point. because nothing felt really clean ... any suggestion/concern is welcome

smoothdeveloper · 2016-04-23T11:10:12Z

@matthid I think the code should now go right into Paket.Core, could you make that happen so I'm not stomping on files being moved?

Once this is done I intend to look at adding a generate-include-scripts command line to paket.exe and do some work on integration tests.

smoothdeveloper · 2016-04-23T14:42:37Z

@matthid I've added generate-include-scripts command in main executable, right now it should only be called if packages are already installed, I think we need some error handling otherwise and I'll be checking for similar checks in other commands.

@forki there is some crude framework detection going on in the code I've added, I think to keep in in this command only for now as it is very rough, but would appreciate if there is any pointer in paket codebase dealing with those aspects.

smoothdeveloper · 2016-04-23T14:43:39Z

src/Paket.Core/ScriptGeneration.fs

+      IncludeScriptsRootFolder     : DirectoryInfo
+      DependentScripts             : FileInfo seq
+      FrameworkReferences          : string seq
+      OrderedRealtiveDllReferences : string seq


typo? OrderedRelativeDllReferences

yep, thanks

matthid · 2016-04-23T14:48:42Z

@smoothdeveloper I added framework-dependencies and moved the thing to paket.core, but I guess you already saw that :) .

smoothdeveloper · 2016-04-23T14:52:15Z

@matthid thanks I overlooked framework dependencies, will pay a look at how it works, thanks for cleaning a bit the mess in ScriptGeneration.fs :)

…ges subfolders

…written

…a list instead of a seq

… set of dependencies * move fst' as an inner function * bit of documentation

* integration tests covering new behaviour (minor) rename function

…ation

… be run.

smoothdeveloper · 2016-05-03T15:01:34Z

Mmh, I need to reproduce this case on a linux / macos machine I guess, I believe the script generation doesn't end up with same scripts on windows vs mono despite the framework is specified.

Could it be that InstallModel gives different results on vanilla dotnet / mono?

@matthid / @forki what do you recommend we do with this test? I don't feel it should hold the feature so I might just Assert.Ignore with a message about that issue.

…ow (only when it fails). The test passes under windows and macos, but not on linux so this needs to be troubleshooted separately and not hold merge of the PR.

smoothdeveloper · 2016-05-04T22:03:47Z

@forki, I've disabled the offending test, it won't work on linux until someone can really step through the code there.

Please check again if this can be merged now.

Thanks!

forki · 2016-05-04T22:34:15Z

I will test this as soon as I have working Internet connection on my pc
again. The Telekom guy is coming on Monday. Sorry for delay. Blame Deutsche
Telekom.
On May 5, 2016 12:03 AM, "Gauthier Segay" notifications@github.com wrote:

@forki https://github.com/forki, I've disabled the offending test, it
won't work on linux until someone can really step through the code there.

Please check again if this can be merged now.

Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1619 (comment)

forki · 2016-05-10T08:27:44Z

docs/content/commands/generate-include-scripts.md

+This will create .csx and .fsx scripts under `paket-files/include-scripts/net45/`, those files can now be 
+used in your scripts without having to bother with the list and order of all dependencies for given package.
+
+Note: this command only works after packages have been restored, please call `paket restore` before using `paket generate-include-scripts` or `paket install` if you just changed your `paket.dependencies` file.


can we make it call restore (in a future PR)?

You mean having the command itself call restore to remove the need from the user?

If you think that's the sensible choice, yes I can make a PR for this.

Similarly, do you have an idea about how it should interact with "garbage collection" story?

I think create script should do a restore.
I think gc should be extended to clean unused load scripts
On May 10, 2016 10:37, "Gauthier Segay" notifications@github.com wrote:

In docs/content/commands/generate-include-scripts.md
#1619 (comment):

@@ -0,0 +1,55 @@
+## Generating include scripts for all Nuget packages
+
+It is possible to generate include scripts for all registered Nuget packages defined in paket.dependencies.
+

[lang=batchfile]

$ paket generate-include-scripts framework net45

+This will create .csx and .fsx scripts under paket-files/include-scripts/net45/, those files can now be
+used in your scripts without having to bother with the list and order of all dependencies for given package.
+
+Note: this command only works after packages have been restored, please call paket restore before using paket generate-include-scripts or paket install if you just changed your paket.dependencies file.

You mean having the command itself call restore to remove the need from
the user?

If you think that's the sensible choice, yes I can make a PR for this.

Similarly, do you have an idea about how it should interact with "garbage
collection" story?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/fsprojects/Paket/pull/1619/files/3eda83abb200591365d4aac47cbe30d908b111ba#r62631374

forki · 2016-05-10T09:08:50Z

Awesome work! Thanks so much. xoxo

and now:

forki · 2016-05-11T11:56:33Z

can you please try to make the test pass on ncrunch? the path is too long...

smoothdeveloper · 2016-05-11T13:05:42Z

Yes I'll rename the folder with all the related tests.

smoothdeveloper · 2016-05-11T13:06:01Z

Any chance to tell ncrunch to work somewhere else?

forki · 2016-05-11T13:12:47Z

maybe, but don't know how

matthid mentioned this pull request Apr 18, 2016

WIP: start to implement a loading script generator. #1613

Closed

4 tasks

smoothdeveloper reviewed Apr 18, 2016
View reviewed changes

matthid reviewed Apr 19, 2016
View reviewed changes

smoothdeveloper reviewed Apr 23, 2016
View reviewed changes

smoothdeveloper and others added 16 commits May 3, 2016 16:11

print out list of bogus frameworks

cdfa4f2

(minor) more accurate symbol name

8adb286

use compare string for groupname (so it matches format used in /packa…

fd6275a

…ges subfolders

don't generate a script if no includes, references or loads would be …

9bb2343

…written

categorize the tests for easier running

aab54bd

add integration test showing the no-script-generation functionality

b3b5917

more tests around file type script generation

6a9ead6

change scriptgen function to use a dedicated result type, and output …

bbb2236

…a list instead of a seq

inline call to remove local

dbf54bb

* add simple unit test to check DoNotGenerate is returned given empty…

cf3f867

… set of dependencies * move fst' as an inner function * bit of documentation

* handle invalid arguments (fail fast)

c622510

* integration tests covering new behaviour (minor) rename function

add Mono.Cecil to list of mergeLibs in build.fsx

2e47f55

add failure message in two unit tests related to include script gener…

8dd29f2

…ation

documentation for generate-include-scripts

abbafd5

better clarification on when the generate-include-scripts command can…

943d832

… be run.

make test independent from DirectoryInfo.GetFiles order

7d3fbc5

smoothdeveloper force-pushed the generate_scripts_v3 branch from ad53d3b to 7d3fbc5 Compare May 3, 2016 14:21

discard simple dependencies generates expected scripts test for n…

3eda83a

…ow (only when it fails). The test passes under windows and macos, but not on linux so this needs to be troubleshooted separately and not hold merge of the PR.

forki reviewed May 10, 2016
View reviewed changes

forki merged commit 3eda83a into fsprojects:v3 May 10, 2016

This was referenced May 10, 2016

Add top level include scripts (for a group) #1680

Closed

Generating include scripts on restore #1681

Closed

Implement a loading script generator. #1619

Implement a loading script generator. #1619

Conversation

matthid commented Apr 18, 2016 • edited Loading

matthid commented Apr 18, 2016

matthid commented Apr 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoothdeveloper commented Apr 18, 2016

smoothdeveloper commented Apr 18, 2016

matthid commented Apr 18, 2016

smoothdeveloper commented Apr 19, 2016

smoothdeveloper commented Apr 19, 2016

smoothdeveloper commented Apr 19, 2016

smoothdeveloper commented Apr 19, 2016

forki commented Apr 19, 2016

Choose a reason for hiding this comment

matthid commented Apr 19, 2016

matthid commented Apr 19, 2016

smoothdeveloper commented Apr 19, 2016

smoothdeveloper commented Apr 21, 2016

baronfel commented Apr 21, 2016

baronfel commented Apr 21, 2016

matthid commented Apr 21, 2016

baronfel commented Apr 21, 2016

matthid commented Apr 21, 2016

smoothdeveloper commented Apr 23, 2016

smoothdeveloper commented Apr 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Apr 23, 2016

smoothdeveloper commented Apr 23, 2016

smoothdeveloper commented May 3, 2016

smoothdeveloper commented May 4, 2016

forki commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forki commented May 10, 2016

forki commented May 11, 2016

smoothdeveloper commented May 11, 2016

smoothdeveloper commented May 11, 2016

forki commented May 11, 2016

matthid commented Apr 18, 2016 •

edited

Loading