Skip to content

Fixes an issue where the binding redirect version is picked from the fir... #422

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

Merged
forki merged 4 commits into from
Dec 5, 2014
Merged

Fixes an issue where the binding redirect version is picked from the fir... #422

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5434895
Fixes an issue where the binding redirect version is picked from the …
Dec 4, 2014
75dcc4b
changed package to library
Dec 4, 2014
dc208d6
undue my last change from p to lib
Dec 4, 2014
4a1f1e9
Correct casing of library
Dec 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/Paket.Core/InstallProcess.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ open System.IO
open System.Collections.Generic
open FSharp.Polyfill
open System.Reflection
open System.Diagnostics

let private findPackagesWithContent (root,usedPackages:HashSet<_>) =
usedPackages
Expand Down Expand Up @@ -95,6 +96,12 @@ let createModel(root, sources,force, lockFile:LockFile) =

extractedPackages

/// Picks the highest version of a package
let private pickHighestPackageVersion packages =
packages
|> Seq.sortBy (fun p -> FileVersionInfo.GetVersionInfo(p).FileVersion)

@mexx mexx Dec 4, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the FileVersion the same as the Assembly.GetName().Version, could them be different?

@TWith2Sugars TWith2Sugars Dec 4, 2014

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FileVersion is just a string while the assembly version is an object .

@mexx mexx Dec 4, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Beside from the types, they could differ in content.
I can annotate an assembly like this:

[<assembly: AssemblyVersionAttribute("0.17.12")>]
[<assembly: AssemblyFileVersionAttribute("0.17.12 special build")>]

With this the ordering can get wrong as well.
We should use the AssemblyName.Version.

|> Seq.last

/// Applies binding redirects for all strong-named references to all app. and web. config files.
let private applyBindingRedirects root extractedPackages =
extractedPackages
Expand All @@ -104,7 +111,8 @@ let private applyBindingRedirects root extractedPackages =
match ref with
| Reference.Library path -> Some path
| _-> None)
|> Seq.distinctBy (fun p -> FileInfo(p).Name)
|> Seq.groupBy (fun p -> FileInfo(p).Name)
|> Seq.map(fun (_,ps) -> pickHighestPackageVersion ps)

@forki forki Dec 4, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the issue is that we're not talking about packages here.
We have libs with conflicting version nos.

@TWith2Sugars TWith2Sugars Dec 4, 2014

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just followed from the extractedPackages and made the assumption p = package

@forki forki Dec 4, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yep sorry that's my fault. Should have made it more explicit. p is a library path in this case.
Could you please fix this with your PR? Thanks

@TWith2Sugars TWith2Sugars Dec 4, 2014

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

all of it or just my changes?

@forki forki Dec 4, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No it's just in the lines you touched.

|> Seq.choose(fun assemblyFileName ->
try
let assembly = Assembly.ReflectionOnlyLoadFrom assemblyFileName
Expand Down