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

Adding support for SpecificVersion attribute on assembly references #2413

Merged
merged 4 commits into from Jun 12, 2017

Conversation

Projects
None yet
3 participants
@batzen
Contributor

batzen commented Jun 11, 2017

This PR adds support for SpecificVersion on assembly references.
This was requested in #2040.

  • Implement feature
  • Fix unit and integration tests (partially done, see below)
  • Add new tests to verify feature

I'm currently having a hard time trying to find out which integration tests i broke with my change as they often fail because either the path is too long (exceeds 260 characters for file or 248 for directory) (solved by moving to c:\paket) or they fail with

Expected: "...argetFrameworkVersion) == 'v4.6.1' Or $(TargetFrameworkVer..."
But was: "...argetFrameworkVersion) == 'v4.5' Or $(TargetFrameworkVersi..."

The tests failing with this error are:

  • #1427 install content
  • #1427 install content once from dependencies file
  • #1427 install content once from dependencies file removes paket tag
  • #1427 install content once from dependencies file stays stable
  • #1427 install content once from dependencies file stays stable 2 installs
  • #1427 install content once from references file
  • #1427 won't install content when content:none

The following tests timeout:

  • #1174 Should find Ninject error

I don't think those fails are caused by my changes, but i can't be sure...

Please tell me if i did anything wrong as this is the first time i am in contact with F#.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jun 11, 2017

Member

can you rebase/merge? Currently master is green so it might solve the spurious issues you encountered (there was a problem with the integration test suite which was solved)...

Member

matthid commented Jun 11, 2017

can you rebase/merge? Currently master is green so it might solve the spurious issues you encountered (there was a problem with the integration test suite which was solved)...

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jun 11, 2017

Member

Nevermind you were faster :)

Member

matthid commented Jun 11, 2017

Nevermind you were faster :)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 11, 2017

Member

Is this flag enabled by default?

Member

forki commented Jun 11, 2017

Is this flag enabled by default?

@batzen

This comment has been minimized.

Show comment
Hide comment
@batzen

batzen Jun 11, 2017

Contributor

The default is True which is the same as omitting it in assembly references.

Contributor

batzen commented Jun 11, 2017

The default is True which is the same as omitting it in assembly references.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 11, 2017

Member

We should not emit it then in default case to reduce impact in csproj

Member

forki commented Jun 11, 2017

We should not emit it then in default case to reduce impact in csproj

@batzen

This comment has been minimized.

Show comment
Hide comment
@batzen

batzen Jun 11, 2017

Contributor

I thought that i should add it to csproj as private is also added by default.
If you don't want it to be added i will remove it if it's not specified or equals true.

Contributor

batzen commented Jun 11, 2017

I thought that i should add it to csproj as private is also added by default.
If you don't want it to be added i will remove it if it's not specified or equals true.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jun 11, 2017

Member

@forki If we get this into paket 5 I guess it's OKisch, because it will already rewrite a lot because of new restriction system, netcore and xamarin support...

Member

matthid commented Jun 11, 2017

@forki If we get this into paket 5 I guess it's OKisch, because it will already rewrite a lot because of new restriction system, netcore and xamarin support...

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 11, 2017

Member
Member

forki commented Jun 11, 2017

@batzen

This comment has been minimized.

Show comment
Hide comment
@batzen

batzen Jun 11, 2017

Contributor

So which way should i go?
Omit it if it's not specified (None) or also omit it if it's specified with the default value of True?

Contributor

batzen commented Jun 11, 2017

So which way should i go?
Omit it if it's not specified (None) or also omit it if it's specified with the default value of True?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 11, 2017

Member
Member

forki commented Jun 11, 2017

@batzen

This comment has been minimized.

Show comment
Hide comment
@batzen

batzen Jun 11, 2017

Contributor

Ok.
Will make the changes accordingly and revert most changes to tests.

Contributor

batzen commented Jun 11, 2017

Ok.
Will make the changes accordingly and revert most changes to tests.

@batzen

This comment has been minimized.

Show comment
Hide comment
@batzen

batzen Jun 11, 2017

Contributor

Should be as expected now + tests are present now too.

Contributor

batzen commented Jun 11, 2017

Should be as expected now + tests are present now too.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 12, 2017

Member

💋

Member

forki commented Jun 12, 2017

💋

@forki forki merged commit d7754c6 into fsprojects:master Jun 12, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 13, 2017

Member

Urgs I forgot to ask for docs and already released it in rc006. Can you please send a PR to the docs as well?

I think it needs to go to https://github.com/fsprojects/Paket/blob/master/docs/content/nuget-dependencies.md#copy_local-settings and https://github.com/fsprojects/Paket/blob/master/docs/content/references-files.md#copy_local-settings

Member

forki commented Jun 13, 2017

Urgs I forgot to ask for docs and already released it in rc006. Can you please send a PR to the docs as well?

I think it needs to go to https://github.com/fsprojects/Paket/blob/master/docs/content/nuget-dependencies.md#copy_local-settings and https://github.com/fsprojects/Paket/blob/master/docs/content/references-files.md#copy_local-settings

@batzen

This comment has been minimized.

Show comment
Hide comment
@batzen

batzen Jun 13, 2017

Contributor

Of course. Didn't know about that. Will get it done till friday.

Contributor

batzen commented Jun 13, 2017

Of course. Didn't know about that. Will get it done till friday.

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