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

Generate include script for each group #1787

Merged
merged 3 commits into from Jul 12, 2016

Conversation

Projects
None yet
4 participants
@smoothdeveloper
Contributor

smoothdeveloper commented Jul 3, 2016

This includes #1786 (I did that to separate code review if we want to merge in two steps)

This PR add generation of a script per group, like @Krzysztof-Cieslak asked for:

paket-files/include-scripts/net45/include.main.group.fsx

the include script contains all the framework and file assemblies resolved in a single pass.

  • generate a script per group
  • write few tests
  • do not include mscorlib and FSharp.Core for .fsx files (factorize the existing code)

smoothdeveloper added some commits Jul 2, 2016

Small refactor of generate-include-scripts internal implementation.
Now scripts are being constructed as a list of ScriptPiece (a simple DU) and then rendered into string written in the resulting file.

This is needed so I have better infrastructure to work on a later feature related to include scripts.
@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jul 3, 2016

Member

Oh nice, this fixes #1680 as well. Actually I could use this right now in my FAKE stuff, so perfect timing :)

Member

matthid commented Jul 3, 2016

Oh nice, this fixes #1680 as well. Actually I could use this right now in my FAKE stuff, so perfect timing :)

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jul 3, 2016

Member

Why generate "one big" script instead of loading the ones we already have?

Member

matthid commented Jul 3, 2016

Why generate "one big" script instead of loading the ones we already have?

@smoothdeveloper

This comment has been minimized.

Show comment
Hide comment
@smoothdeveloper

smoothdeveloper Jul 3, 2016

Contributor

@matthid few reasons:

  • I suspect the compiler / tooling to handle this better (Microsoft/visualfsharp#1311) which is basically what prompted me to go this route (and help investigate the issue)
  • it should be faster, it won't parse dozen of files and need less deduplication to be executed
  • it is also more convenient to have a single list of all of those references when user wants to make a workaround in his own include script in case this one doesn't work out of the box (most likely due to tooling bug)
  • more user friendly when I "go to definition" on the script and can see all of it's beauty without browsing a web of small files

since the file is generated also and code to implement that is rather small, I don't think we should bother taking another route.

What do you think?

Thanks for feedback!

Contributor

smoothdeveloper commented Jul 3, 2016

@matthid few reasons:

  • I suspect the compiler / tooling to handle this better (Microsoft/visualfsharp#1311) which is basically what prompted me to go this route (and help investigate the issue)
  • it should be faster, it won't parse dozen of files and need less deduplication to be executed
  • it is also more convenient to have a single list of all of those references when user wants to make a workaround in his own include script in case this one doesn't work out of the box (most likely due to tooling bug)
  • more user friendly when I "go to definition" on the script and can see all of it's beauty without browsing a web of small files

since the file is generated also and code to implement that is rather small, I don't think we should bother taking another route.

What do you think?

Thanks for feedback!

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jul 3, 2016

Member

Yeah I don't have a strong opinion on this, just curiosity. I think generating one file is fine.

One thing I noticed (if I even saw that correctly) is that you currently don't use the ordered package list. This might lead to errors for dependencies across packages.

Member

matthid commented Jul 3, 2016

Yeah I don't have a strong opinion on this, just curiosity. I think generating one file is fine.

One thing I noticed (if I even saw that correctly) is that you currently don't use the ordered package list. This might lead to errors for dependencies across packages.

@smoothdeveloper

This comment has been minimized.

Show comment
Hide comment
@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jul 3, 2016

Member

Ah I didn't noticed that you are running the function over the complete list of assemblies for the group. Yeah this should work.

Member

matthid commented Jul 3, 2016

Ah I didn't noticed that you are running the function over the complete list of assemblies for the group. Yeah this should work.

@isaacabraham

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Jul 3, 2016

Contributor

You also don't want to load the same assemblies in twice, so a single script without duplicates might be preferable.

Contributor

isaacabraham commented Jul 3, 2016

You also don't want to load the same assemblies in twice, so a single script without duplicates might be preferable.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 12, 2016

Member

is tis ready to merge?

Member

forki commented Jul 12, 2016

is tis ready to merge?

@smoothdeveloper

This comment has been minimized.

Show comment
Hide comment
@smoothdeveloper

smoothdeveloper Jul 12, 2016

Contributor

@forki the feature is working but it doesn't have specific integration/unit tests yet, and there are probably bugs that we have yet to find from usage.

I'm ok having it merged (useful feature) and to work on adding test in separate PR.

Contributor

smoothdeveloper commented Jul 12, 2016

@forki the feature is working but it doesn't have specific integration/unit tests yet, and there are probably bugs that we have yet to find from usage.

I'm ok having it merged (useful feature) and to work on adding test in separate PR.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 12, 2016

Member

Looks like one of the related integration tests is broken

Member

forki commented Jul 12, 2016

Looks like one of the related integration tests is broken

@forki forki merged commit 3bc9619 into fsprojects:master Jul 12, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 12, 2016

Member

can you please take a look at master. I have 3 broken tests now

Member

forki commented Jul 12, 2016

can you please take a look at master. I have 3 broken tests now

@smoothdeveloper

This comment has been minimized.

Show comment
Hide comment
@smoothdeveloper

smoothdeveloper Jul 12, 2016

Contributor

@forki, I'm on it I'll have a fix shortly.

Contributor

smoothdeveloper commented Jul 12, 2016

@forki, I'm on it I'll have a fix shortly.

@smoothdeveloper smoothdeveloper changed the title from [WIP] Generate include script for each group to Generate include script for each group Oct 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment