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

Fixes 3352 - copy local has no effect when opening project in vs2017 #3356

Merged
merged 5 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@atlemann
Contributor

atlemann commented Sep 11, 2018

Fixes: #3352

Setting <ExcludeAssets>runtime</ExcludeAssets> in the .paket.props file for packages with copy_local: false to avoid VS2017 changing the props.assets.json when loading the project.

Tweaked the result of combining copy_local flags from lock and references file:

  • Setting only copy_local: true returns Some true which results in PrivateAssets=All
  • Combinations of copy_local: true and copy_local: false results in either Some false or None which results in either ExcludeAssets=runtime or no flags at all.

Moved the copy_local setting to element 5 in the .paket.resolved file in the obj folder and updated paket.Restore.targets to read the 5th element instead.

atlemann added some commits Sep 11, 2018

Add ExcludeAssets=runtime in .paket.props when copy_local: false,
to avoid VS2017 to change the project.assets.json when opening a project
Excract function for combining copy_local from lock and references files
@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 12, 2018

Contributor

So, what's up with the Travis build? Does it break from time to time or did my changes break something?

Contributor

atlemann commented Sep 12, 2018

So, what's up with the Travis build? Does it break from time to time or did my changes break something?

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 12, 2018

Contributor

Why I'm asking about PrivateAssets is that I have a scenario where I have copy_local: false for some packages in the paket.dependencies file and copy_local: true for some of them in some paket.references files (in this case for unit-test projects). This results in PrivateAssets=All being set for those packages. Why is the copy_local flag used to set PrivateAssets=All when it is ExcludeAssets=runtime that enables the expected behavior?

Contributor

atlemann commented Sep 12, 2018

Why I'm asking about PrivateAssets is that I have a scenario where I have copy_local: false for some packages in the paket.dependencies file and copy_local: true for some of them in some paket.references files (in this case for unit-test projects). This results in PrivateAssets=All being set for those packages. Why is the copy_local flag used to set PrivateAssets=All when it is ExcludeAssets=runtime that enables the expected behavior?

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 12, 2018

Contributor

Other than the PrivateAssets thing, the changes committed fixes my issue. Should I remove the WIP and open another issue with the PrivateAssets thing instead?

Contributor

atlemann commented Sep 12, 2018

Other than the PrivateAssets thing, the changes committed fixes my issue. Should I remove the WIP and open another issue with the PrivateAssets thing instead?

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 12, 2018

Contributor

I guess this is why: #2951

Contributor

atlemann commented Sep 12, 2018

I guess this is why: #2951

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 12, 2018

Contributor

Maybe I could change the code to only consider copy_local: true in the paket.depencencies file to return true to the .paket.props file. If copy_local: true is set in the paket.references file, I could just return false which wouldn't set either PrivateAssets or ExcludeAssets, which for me is the expected behavior. It would be kind of confusing though.

Contributor

atlemann commented Sep 12, 2018

Maybe I could change the code to only consider copy_local: true in the paket.depencencies file to return true to the .paket.props file. If copy_local: true is set in the paket.references file, I could just return false which wouldn't set either PrivateAssets or ExcludeAssets, which for me is the expected behavior. It would be kind of confusing though.

@atlemann atlemann changed the title from [WIP] Fixes 3352 - copy local has no effect when opening project in vs2017 to Fixes 3352 - copy local has no effect when opening project in vs2017 Sep 13, 2018

Set PrivateAssets=All if only copy_local:true is set for a package in…
… either lock or references file

Separated the PrivateAssets and CopyLocal settings in .paket.resolved
Added CopyLocal tag in paket.Restore.targets which reads the new column
@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 14, 2018

Contributor

I'm done with this one as it fixes the problem I had with opening projects in VS2017 with copy_local: false.

Contributor

atlemann commented Sep 14, 2018

I'm done with this one as it fixes the problem I had with opening projects in VS2017 with copy_local: false.

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 14, 2018

Contributor

Unless I have to change the PaketPropsVersion in RestoreProcess.fs since I'm now editing the .paket.props file

Contributor

atlemann commented Sep 14, 2018

Unless I have to change the PaketPropsVersion in RestoreProcess.fs since I'm now editing the .paket.props file

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 17, 2018

Contributor

@forki @matthid Sorry for bothering you, but these changes fixes the issues I got when converting a C# solution with 180 projects (I know, I want to hang myself) to new SDK format. We're using the copy_local option quite extensively and would need this fix or something similar since VS2017 is changing the runtime entries in the project.assets.json when it loads the projects. This results in the .dlls with copy_local: false to be copied to the bin folders anyway.

Is there another way to fix this I should try instead?
Do you have some advice on how to add a test for this?

Running build.sh is all green locally, so I'm not sure why the builds are failing here.

Contributor

atlemann commented Sep 17, 2018

@forki @matthid Sorry for bothering you, but these changes fixes the issues I got when converting a C# solution with 180 projects (I know, I want to hang myself) to new SDK format. We're using the copy_local option quite extensively and would need this fix or something similar since VS2017 is changing the runtime entries in the project.assets.json when it loads the projects. This results in the .dlls with copy_local: false to be copied to the bin folders anyway.

Is there another way to fix this I should try instead?
Do you have some advice on how to add a test for this?

Running build.sh is all green locally, so I'm not sure why the builds are failing here.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Sep 17, 2018

Member

@atlemann Thanks for the PR.

Regarding the Code: I think there should be a built-in to get the "package settings" after considering the paket.references file. So I'm a bit hesitant about doing that manually.

Regarding the Test-Suite: Yes it is probably broken again. I assume at least appveyor should be green again after adding the netcoreapp2.2 changes to the integration test suite (/cc @forki )

Member

matthid commented Sep 17, 2018

@atlemann Thanks for the PR.

Regarding the Code: I think there should be a built-in to get the "package settings" after considering the paket.references file. So I'm a bit hesitant about doing that manually.

Regarding the Test-Suite: Yes it is probably broken again. I assume at least appveyor should be green again after adding the netcoreapp2.2 changes to the integration test suite (/cc @forki )

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 17, 2018

Contributor

Um, not sure I follow what you mean by built-in?

Contributor

atlemann commented Sep 17, 2018

Um, not sure I follow what you mean by built-in?

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 19, 2018

Contributor

@matthid Is there anything I can change to make it mergable? I basically have to postpone switching to new SDK format due to this.

Contributor

atlemann commented Sep 19, 2018

@matthid Is there anything I can change to make it mergable? I basically have to postpone switching to new SDK format due to this.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Sep 19, 2018

Member

please merge with master

Member

forki commented Sep 19, 2018

please merge with master

@atlemann

This comment has been minimized.

Show comment
Hide comment
@atlemann

atlemann Sep 20, 2018

Contributor

At least the Appveyor build succeeded again now

Contributor

atlemann commented Sep 20, 2018

At least the Appveyor build succeeded again now

@forki forki merged commit 9a68591 into fsprojects:master Sep 20, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Sep 20, 2018

Member

thanks!

Member

forki commented Sep 20, 2018

thanks!

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