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: start to implement a loading script generator. #1613

Closed
wants to merge 9 commits into from

Conversation

matthid
Copy link
Member

@matthid matthid commented Apr 16, 2016

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
  • Integrate in Paket or a separate project

else
let libDir = Path.Combine (packageFolder, "lib")
// TODO: Replace with some intelligent code to use the correct framework dlls,
// Generate multiple include scripts per-framework.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider having precompiler conditionals instead? like #if MONO etc. or at least have a top level script with those loading platform specific scripts for those who want to use that instead of picking a platform dependent script.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was more thinking of NET45, NET40, PROFILE111 and so forth, not a MONO one. I think writing a single script with #ifs instead of multiple scripts per runtime is more difficult. But all the information we need should be available in paket.core somewhere :).

I don't know if #ifs are good from an end-user perspective as they would need to define their platform on the command line (because AFAIK there are no symbols predefined which we could use?). On the other side most users will probably understand what Include_packagename_net45.fsx or Include_packagename_coreclr.fsx means

Copy link
Contributor

Choose a reason for hiding this comment

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

@matthid agreed with defines being a bit of a pain for end-users, I propose we create such structure:

packages/includes/{platform-name}/{packagename}.fsx

Reason is that I don't think it is good practice to change packages subfolders which are managed by paket and might conflict at later point with some logic.

For now, {platform-name} subfolder will not be used (see the hardcoded framework list I've put in the code) but we should make the code suitable for introducing this later (I made a first attempt at this).

Copy link
Member Author

Choose a reason for hiding this comment

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

and might conflict at later point with some logic.

I think that really depends on how we decide to integrate this. If its integrated into paket itself I don't think this is a problem.

smoothdeveloper referenced this pull request in smoothdeveloper/Paket Apr 17, 2016
* getDllFilesWithinPackage now takes a targetPredicate which allows to identify the correct Paket.LibFolder

* add weightTargetProfiles and makeTargetPredicate functions

weightTargetProfiles takes a list of Paket.FrameworkIdentifier (in order of preference) and a list of Paket.TargetProfile and weight each profile (lower weight is considered favored)

makeTargetPredicate is helper function to make a suitable predicate for getDllFilesWithinPackage given list of prefered Paket.FrameworkIdentifier
matthid and others added 9 commits April 17, 2016 23:16
* add a ScriptBuilder group to paket.dependencies, right now I need Mono.Cecil and QuickGraph to get going with that work

* minor changes to LoadingScriptsGenerator.fs to have more explicit requirements in functions (depFile -> lockFile because we intend it to point to paket.lock, changed strings to DirectoryInfo and FileInfo for a bit of clarity at this point)

* changes to Paket.LoadingScripts.fsproj

* changes to Script.fs: add getDllFilesWithinPackage and computePackageTopologyGraph

getDllFilesWithinPackage is used to figure out order of dlls within a single package (for example QuickGraph contains several assemblies, this is used to figure out the order those should be referenced)

computePackageTopologyGraph is used to give the top level package names one given package depends on in order suitable for writing references recursively
* getDllFilesWithinPackage now takes a targetPredicate which allows to identify the correct Paket.LibFolder

* add weightTargetProfiles and makeTargetPredicate functions

weightTargetProfiles takes a list of Paket.FrameworkIdentifier (in order of preference) and a list of Paket.TargetProfile and weight each profile (lower weight is considered favored)

makeTargetPredicate is helper function to make a suitable predicate for getDllFilesWithinPackage given list of prefered Paket.FrameworkIdentifier
…r, pass those down to lower level functions

* migrate and integrate weightTargetProfiles, makeTargetPredicate and getDllFilesWithinPackage in generateFSharpScript
…nto HEAD

Conflicts:
	paket.dependencies
	paket.lock
@smoothdeveloper
Copy link
Contributor

I tried to rebase on latest paket master, but somehow couldn't figure out how to push the result to this branch.

If you have some details on how I should attempt this (I guess we will need to do it a few more times or even rebase on v3 eventually).

@matthid
Copy link
Member Author

matthid commented Apr 17, 2016

You probably need to force push, changing the target branch (to v3) is not possible anyway (we need a new PR for this).

@smoothdeveloper
Copy link
Contributor

smoothdeveloper commented Apr 17, 2016

@matthid I think Handle dependencies within the package can be checked but it is worth for you to look it up, you can check with QuickGraph which has several assemblies.

That being said, we need to plan with @forki if we can ultimately rely on Mono.Cecil (274kb) + QuickGraph (314kb) or if we should plan for a separate executable.

I assume both those dependencies could be valuable to reduce amount of logic / code in other places withing Paket itself.

@matthid
Copy link
Member Author

matthid commented Apr 18, 2016

Closing this in favor of #1619

@matthid matthid closed this Apr 18, 2016
@matthid matthid deleted the generate_scripts branch April 24, 2016 20:22
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.

2 participants