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

don't not add duplicated references #3107

Merged
merged 4 commits into from Mar 14, 2018

Conversation

Projects
None yet
3 participants
@0x53A
Contributor

0x53A commented Mar 10, 2018

HACK: workaround for #2811

  • DO remove FW-References which are already referenced by the user
  • DO NOT remove package references, where the user already has a reference
    Example where this is relevant:
  1. Pre-existing reference to the FW-Assembly System.IO.Compression
  2. user adds nuget package https://www.nuget.org/packages/System.IO.Compression/

Before:

  • the old reference is kept, the new one not added
  • no feedback during compile, assembly mismatch at runtime

After:

  • new reference is added
  • duplicated reference, warning at compile time
  • msbuild automatically takes the higher version, everything is good
  • user can manually remove old reference to silence warning

0x53A added some commits Mar 10, 2018

don't not add duplicated references
HACK: workaround for #2811
 * DO remove FW-References which are already referenced by the user
 * DO NOT remove package references, where the user already has a reference
Example where this is relevant:
1) Pre-existing reference to the FW-Assembly System.IO.Compression
2) user adds nuget package https://www.nuget.org/packages/System.IO.Compression/

Before:
 * the old reference is kept, the new one not added
 * no feedback during compile, assembly mismatch at runtime

After:
 * new reference is added
 * duplicated reference, warning at compile time
 * msbuild automatically takes the higher version, everything is good
 * user can manually remove old reference to silence warning
@forki

This comment has been minimized.

Member

forki commented Mar 11, 2018

@matthid

matthid requested changes Mar 11, 2018 edited

For my part I'd say this change (if we want to do it) needs to go to a different place.

Edit: TO clarify the "different" place is probably ProjectFile.fs

// Example where this is relevant:
// 1) Pre-existing reference to the FW-Assembly System.IO.Compression
// 2) user adds nuget package https://www.nuget.org/packages/System.IO.Compression/
//|> mapCompileLibReferences (Set.filter (fun reference -> Set.contains reference.Name references |> not))

This comment has been minimized.

@matthid

matthid Mar 11, 2018

Member

I'm not sure this is the place to do it. InstallModel is / or should be independent from msbuild/project file quirks

This comment has been minimized.

@0x53A

0x53A Mar 14, 2018

Contributor

filterReferences is called from ProjectFile.fs:
image

This is the only place that calls it.

So I could either

  1. keep it as-is
  2. split filterReferences into filterReferences and filterFrameworkReferences, and just never call the first one
  3. rename filterReferences to filterFrameworkReferences

This comment has been minimized.

@matthid

matthid Mar 14, 2018

Member

2 and 3 are fine by me. Basically it's less about where it is used but that the InstallModel-API speaks for itself and is consistent in its naming (it is already hard to understand as it is)

@matthid

This comment has been minimized.

Member

matthid commented Mar 11, 2018

Just some notes on this:

  • I'm pretty sure we did the current behavior on purpose. It is not something introduced by accident. (I guess there exists a documentation/issue around this - it would be nice to find that again).
  • I'm fine with releasing your suggestion and to look what happens
  • Another "solution" would be to let paket warn when we don't add something because of an existing fraemwork reference
@0x53A

This comment has been minimized.

Contributor

0x53A commented Mar 11, 2018

Yeah, I don't like this "solution" either.

  1. imo the status quo is almost worst case - it is broken silently without any feedback.

  2. nuget does remove and replace them.

  3. I think most people will just ignore a warning at install time, because there are already so many.

  4. that's why I would prefer to either keep old and add new, which will create a warning at compile time, which is wayyy more visible to the user ...

  5. ... or just do the same as nuget and remove them.

@matthid

This comment has been minimized.

Member

matthid commented Mar 11, 2018

I'm ok either way (removing or always adding). The only thing here is that the logic needs to be moved to ProjectFile instead of InstallModel IMHO.

@0x53A

This comment has been minimized.

Contributor

0x53A commented Mar 11, 2018

Ok, Will do in the evening. Thank you.
Edit: added comment

@0x53A 0x53A changed the title from [WIP / CI] don't not add duplicated references to don't not add duplicated references Mar 14, 2018

@forki

This comment has been minimized.

Member

forki commented Mar 14, 2018

done?

@0x53A

This comment has been minimized.

Contributor

0x53A commented Mar 14, 2018

@forki

This comment has been minimized.

Member

forki commented Mar 14, 2018

@matthid

This comment has been minimized.

Member

matthid commented Mar 14, 2018

@0x53A Thanks :)

@forki forki merged commit 3c24d3f into fsprojects:master Mar 14, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment