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.

Sign up for GitHub

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

Jump to bottom

Suggestions for load script generation #1943

Closed
dsyme opened this issue Oct 6, 2016 · 24 comments
Closed

Suggestions for load script generation #1943

dsyme opened this issue Oct 6, 2016 · 24 comments

Comments

@dsyme
Copy link
Contributor

dsyme commented Oct 6, 2016

@sylvanc and I have been using the amazing package-load-script-generation feature. It's fantastic

Here are some suggestions for how it can be adjusted

  • Always generate the package load scripts by default. They are just too useful to not do it
  • Always generate by assuming the latest .NET Framework by default (net462) unless told otherwise.
  • Always generate "paket-refs.fsx" (or "paket-refs.csx") next to paket.dependencies that references all the package loads.
  • If there is a non-main group of references then generate paket-build-refs.fsx
  • Never generate a #r on FSharp.Core. I think @tpetricek is adding a separate issue on this.

This would make the feature really, really usable. Right now putting things deep in paket-files makes things hard.

thanks
don

cc @dungpa
@matthid
Copy link
Member

matthid commented Oct 6, 2016

I agree with most, actually I proposed #1681 and #1680.

Defaulting to latest framework sounds like a sane default, however we might not have all dependencies available when framework restrictions are in place. Therefore I'd rather default to the framework restriction we have in place (paket.dependencies) and fall back to latest full framework if there is none.

@dsyme
Copy link
Contributor Author

dsyme commented Oct 6, 2016

Therefore I'd rather default to the framework restriction we have in place (paket.dependencies) and fall back to latest full framework if there is none.

Yes, that

@dsyme
Copy link
Contributor Author

dsyme commented Oct 6, 2016

Yes, #1681 and #1680 cover most of this. +2 for those :)

@forki
Copy link
Member

forki commented Oct 6, 2016

/cc @smoothdeveloper

@smoothdeveloper
Copy link
Contributor

@dsyme excluding FSharp.Core should be easy, could you share the contents of your paket.dependencies where you hit that issue and I'll give it a look (I never noticed it occurring but maybe I used nuget packages which don't rely on that package).

Paket/src/Paket.Core/ScriptGeneration.fs

Line 98 in f5f509a

| "mscorlib" ->

Regarding #1681, I'm not sure it can be done by default, the implementation is kind of costly, on huge paket.dependencies file I have here, it takes almost 30 sec to generate all the current scripts, I'm sure it could be made more efficient with few tricks but I don't see the cost coming close to 0, we have to analyze assembly metadata to figure out correct order of stuff and I don't think there is a shortcut for that.

Paket/src/Paket.Core/ScriptGeneration.fs

Line 46 in f5f509a

let known = dllFiles |> Seq.map (fun a -> a.FullName) |> Set.ofSeq

I'll try to skip that if the list of dll is only one though, that might be most of the actual cost in many cases.

There is also the risk that paket install would fail because of something failing in script generation (one likely reason would be a faulty nuget package with assemblies which make Mono.Cecil fail), which I think would be bad for most users (which aren't using scripts), would we prefer to handle such error with a warning or should we consider an extra option in paket.dependencies?

@smoothdeveloper
Copy link
Contributor

Regarding paket-refs.fsx I'd recommend keeping it under paket-files because paket.dependencies is supposed to be at the root and making that folder more crowded (by default) doesn't seem too principled to me.

I'm thinking that file should #load all the groups scripts under their current location, the only thing which makes me think is that consecutive calls to paket generate-include-scripts with various values of framework would erase that same file, and we would have to pick one framework if the command is invoked with several frameworks (I'm not clear there is such thing as "highest framework version", in the context of different platforms).

I've implemented the optimization to reduce time it takes to generate the scripts: #1945

@isaacabraham
Copy link
Contributor

Having it at the root level just makes it more discoverable and easier to call - one less thing to think about.

@tpetricek
Copy link
Member

@smoothdeveloper For example of paket.dependencies that causes the FSharp.Core trobles, see #1942 (just add Suave).

I'm slightly in favour of file in the root folder too. Could this perhaps be disabled by some option in paket.dependencies? (It could just be #load "paket-files/.../main.fsx" so that it is a simple proxy that does not do anything.

@forki
Copy link
Member

forki commented Dec 2, 2016

@smoothdeveloper IIRC we talked about adding a switch into the deps file to enable script generation. Are you interested in doing this? I think it would help a lot for scenarios like: https://notebooks.azure.com/library/fsharp-templates/html/fsharp-data-usa-states-example.ipynb

@cloudRoutine
Copy link
Member

root directory pollution is becoming an increasing problem as every CI, editor, extension, build tool, and their father, mother, sister and brother has decided they must be at the root to work. I wish people would start adopting the convention that all of this could go in /.config/ at root instead.

Let's not contribute to this issue by default.

@isaacabraham while /paket-files/include-scripts/net46/include.main.group.fsx is definitely onerous wouldn't something like /load-scripts/main.fsx or /paket-files/load.fsx or /paket-files/load-refs.fsx be easy enough?

As far as discoverability goes, an easy way to let people know where the load script is located

λ» paket generate-load-scripts
Paket version 4.0.0-alpha030
generating scripts for framework net46
+ load script generated @
+ ./load-scripts/main.fsx

is just to tell them where the script was generated

(I'm not clear there is such thing as "highest framework version", in the context of different platforms).

@smoothdeveloper looking backward I'm not sure how to approach it, but moving forward the netstandard library version compatibility is the way to set this

But I do agree that at the very least the current convention should really be changed
(maybe for paket4 so it can be breaking 😈 )

First and foremost why are they called include scripts? It's not a term that's used in common F# or .Net parlance, in oth .fsx and .csx files you #load scripts so why not just call it generate-load-scripts?

/paket-files/include-scripts/net46/include.fsharp.compiler.service.fsx

^ why does it need to say include twice?

^ this is already kind of a mess, and as more people start using netcore it's only going to get worse.

Some conventions that could clean this up would be:

  • a specific directory for load scripts like /load-scripts/ at repo root
  • don't put the framework in path of the default scripts generated
  • generate the default script by using the lockfile to determine the lowest compatible framework to load the entire group (unless this has some problems I'm overlooking?)
  • add the framework that's being used by default to the print message for the default generated scripts
Loaded group Main, framework - net46
  • put the load scripts for each group in /load-scripts/ e.g.
/load-scripts/main.fsx
/load-scripts/group1.fsx
/load-scripts/group2.fsx
  • put the load scripts for specific packages in a group in a directory with the same name as that group
/load-scripts/main.fsx
/load-scripts/main/fsharp.compiler.service.fsx
/load-scripts/main/system.buffers.fsx
/load-scripts/main/system.collections.fsx
  • print warning messages when some packages in a group can't have a load script generated for a particular framework
  • print warning/error message when no load scripts can be generated for a particular framework
  • don't create a directory for a framework when no load scripts could be generated for it
  • when generating load scripts for frameworks other then the default put them in a subdirectory with the framework name in the path following the same convention as the defaults
/load-scripts/net40/main.fsx
/load-scripts/net40/main/fsharp.compiler.service.fsx
/load-scripts/net40/group1.fsx
/load-scripts/net40/group2.fsx

@smoothdeveloper
Copy link
Contributor

@forki thanks for the reminder, I'm going to take a look at it, is generate-include-scripts: on the option we want for now?

@cloudRoutine I agree with mostly all you said, maybe we can break / fix this for paket 4.

The root folder pollution is a concern and a load-scripts folder would still contribute to it IMHO, can we somehow keep stuff under paket-files?

@cgravill cgravill mentioned this issue Dec 19, 2016
Merged
@dsyme
Copy link
Contributor Author

dsyme commented Jan 17, 2017

@smoothdeveloper IIRC we talked about adding a switch into the deps file to enable script generation.

That would be great!

@smoothdeveloper
Copy link
Contributor

I need to come back to that, there is a nasty dependency order in the code though which is going to be painful to integrate this change.

@smoothdeveloper
Copy link
Contributor

@dsyme there is a PR #2113 which implements this feature for the v4 alpha, there is also a simple unit -> unit method which was added to the public API's Dependencies object to make it easy to call it that way.

I'll look at doing the (breaking) changes related to folder naming in separate PR at a later time.

@cgravill cgravill mentioned this issue Jan 26, 2017
Closed
@smoothdeveloper
Copy link
Contributor

Regarding load-scripts folder organization, so my stance is:

  • I don't like having such folder at the root, it should stay in paket-files, paket is already using .paket, packages (same as nuget) and paket-files, adding one is really not principled as each tool has this kind of requirement, only thing I think would make sense would be to have a .fsharp folder and have that being an initiative for all fsharp tooling to deal with
  • would paket-files/load-scripts/group.main.fsx and paket-files/load-scripts/libraryname.fsx work? (I want to avoid the odd case of group named the same as a library)
  • would paket-files/load-scripts/net45/ be ok for platform specific (by default it would not generate it there, only in paket-files/load-scripts/, and then platform subfolders based on arbitrary invocation)

Please review the discussion and provide feedback, I'll try to implement the changes once we have cohesion on the topic.

Thanks

@matthid
Copy link
Member

matthid commented Feb 3, 2017

@smoothdeveloper if I could change it today with breaking changes I would like to have the following:

/projectRoot
/projectRoot/paket.dependencies
/projectRoot/Project/paket.references
/projectRoot/.paket/paket.lock
/projectRoot/.paket/packages/FSharp.Core
/projectRoot/.paket/load-scripts/...
/projectRoot/.paket/paket-files/...

Maybe paket.lock should stay in the root directory to better describe the philosophy...
You obviously want to gitignore the packages folder, the load-scripts and paket-files "might" be checked it (to track changes for example), but generally should not be checked-in

@baronfel
Copy link
Contributor

baronfel commented Feb 3, 2017 via email

@smoothdeveloper
Copy link
Contributor

smoothdeveloper commented Feb 3, 2017 •
edited
Loading

I like using .paket the most and .paket/load seems good.

#load ".paket/load/fsharp.data.fsx"

@smoothdeveloper smoothdeveloper mentioned this issue Feb 8, 2017
Merged
@smoothdeveloper
Copy link
Contributor

I've put a PR #2137 to address remarks regarding script names and location.

Note that I've had to keep the nested folder for cases where we specify frameworks, it will generate under the "root" folder (.paket/load) in only those cases:

  • we don't specify any framework from command line
  • when generate_load_scripts is defined in paket.dependencies and framework resolve to only a single version (I believe this means constraint with =)

In practice, generating scripts for different frameworks is useful, many nuget packages have different set of deps/transitive deps according to the framework.

Please share suggestions, if you have any, about a way to consistently resolve a default framework in order to always have scripts generated in root folder.

@smoothdeveloper
Copy link
Contributor

The changes have been released in latest paket 4 alpha release:

  • files have moved to .paket/load folder
  • the include. file name prefix is gone
  • when we can assume a default framework, we don't generate a subfolder with framework name (in practice that mean having framework: single-framework-here in the paket.dependencies file)

@smoothdeveloper
Copy link
Contributor

I'm using this in my script and wonder if there is a good reason we print "library name loaded". This seems a bit heavy handed and makes using the standard output of scripts less usable due to spurious messages.

Would anyone protest if I remove those messages? (

Paket/src/Paket.Core/ScriptGeneration.fs

Line 138 in 9afb18d

| xs -> List.append xs [ PrintStatement (sprintf "%s Loaded" packageName) ] |> Generate
).

@dsyme
Copy link
Contributor Author

dsyme commented Feb 25, 2017

@smoothdeveloper I noticed this too. I do think that should be removed.

@smoothdeveloper
Copy link
Contributor

@dsyme, ok removing those

@matthid
Copy link
Member

matthid commented Aug 20, 2017

I think we have solved this. Please let us know if you have further ideas or I missed something (maybe separate issues make sense)

@matthid matthid closed this as completed Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@forki @smoothdeveloper @tpetricek @baronfel @cloudRoutine @isaacabraham @matthid @dsyme