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

Breaking visual basic projects on paket update #1226

Closed
Rcomian opened this Issue Nov 16, 2015 · 16 comments

Comments

Projects
None yet
3 participants
@Rcomian

Rcomian commented Nov 16, 2015

Using paket v2.25.2.0

When upgrading a visual studio project, it moved an import statement to the end of the file:

<Import Project="$(MSBuildToolsPath)\Microsoft.VisualBasic.targets" />

When this import is after the Reference sections, the references are not loaded. As a result, all paket references are effectively removed from the build.

Moving this Import statement further up the file, before the references, fixed the project.

Fortunately, running update again does not appear to automatically break it again.

The move was part of a block move, where a bunch of included files were moved to the end of the file, along with this import statement.

@Rcomian Rcomian changed the title from Breaking visual studio projects on paket upgrade to Breaking visual basic projects on paket upgrade Nov 16, 2015

@Rcomian Rcomian changed the title from Breaking visual basic projects on paket upgrade to Breaking visual basic projects on paket update Nov 16, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

@theggelund in #1219 you said the targets should be last stuff in the proj files. seems that#s not correct. what should we do?

Member

forki commented Nov 16, 2015

@theggelund in #1219 you said the targets should be last stuff in the proj files. seems that#s not correct. what should we do?

@Rcomian

This comment has been minimized.

Show comment
Hide comment
@Rcomian

Rcomian Nov 16, 2015

The statement that was moved was not part of any nuget package. Can we exclude those and not move them?

Rcomian commented Nov 16, 2015

The statement that was moved was not part of any nuget package. Can we exclude those and not move them?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

we probably can.

Member

forki commented Nov 16, 2015

we probably can.

@forki forki added the bug label Nov 16, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

do you have the csproj file for me before the change?

Member

forki commented Nov 16, 2015

do you have the csproj file for me before the change?

@theggelund

This comment has been minimized.

Show comment
Hide comment
@theggelund

theggelund Nov 16, 2015

@forki I said that targets that are included in a nuget package (build folder) should be put last, not all targets in a file. Ordering of stuff in msbuild files are important

theggelund commented Nov 16, 2015

@forki I said that targets that are included in a nuget package (build folder) should be put last, not all targets in a file. Ordering of stuff in msbuild files are important

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

yes. the big question is how can we detect where we want to put our paket references and paket.targets

Member

forki commented Nov 16, 2015

yes. the big question is how can we detect where we want to put our paket references and paket.targets

@Rcomian

This comment has been minimized.

Show comment
Hide comment
@Rcomian

Rcomian Nov 16, 2015

I do, but since we use a lot of internal packages and code I'm not sure how much use they'll be.

BEFORE_NE.SequenceManager.vbproj.txt
AFTER_NE.SequenceManager.vbproj.txt

The AFTER project can be fixed solely by moving the Include line back above the references.

Rcomian commented Nov 16, 2015

I do, but since we use a lot of internal packages and code I'm not sure how much use they'll be.

BEFORE_NE.SequenceManager.vbproj.txt
AFTER_NE.SequenceManager.vbproj.txt

The AFTER project can be fixed solely by moving the Include line back above the references.

@Rcomian

This comment has been minimized.

Show comment
Hide comment
@Rcomian

Rcomian Nov 16, 2015

Fix looks good - just moving the imports that are controlled by paket.
Can you let me know when there's a build I can try this out on?

Rcomian commented Nov 16, 2015

Fix looks good - just moving the imports that are controlled by paket.
Can you let me know when there's a build I can try this out on?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

I'm still trying to reproduce the error, but the latest version has a fix that might help. Could you please retry with latest paket?

Member

forki commented Nov 16, 2015

I'm still trying to reproduce the error, but the latest version has a fix that might help. Could you please retry with latest paket?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

(and then it's time to cleanup that mess in that function ;-) )

Member

forki commented Nov 16, 2015

(and then it's time to cleanup that mess in that function ;-) )

@Rcomian

This comment has been minimized.

Show comment
Hide comment
@Rcomian

Rcomian Nov 16, 2015

Hmm, no, as of 2.25.3.0, it's now leaving the resource files in place, which I like, but the include is still being moved to the end.

AFTER_2.25.3.0.NE.SequenceManager.vbproj.txt

Looking at the code, I'm not sure why that is. I've tried this twice to make sure.

Rcomian commented Nov 16, 2015

Hmm, no, as of 2.25.3.0, it's now leaving the resource files in place, which I like, but the include is still being moved to the end.

AFTER_2.25.3.0.NE.SequenceManager.vbproj.txt

Looking at the code, I'm not sure why that is. I've tried this twice to make sure.

@Rcomian

This comment has been minimized.

Show comment
Hide comment
@Rcomian

Rcomian Nov 16, 2015

And again, moving the include back to its original location (just under the resources, just before the references) works fine.

Rcomian commented Nov 16, 2015

And again, moving the include back to its original location (just under the resources, just before the references) works fine.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

ok. so it's a off by one issue ;-)

Member

forki commented Nov 16, 2015

ok. so it's a off by one issue ;-)

forki added a commit that referenced this issue Nov 16, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

so seems I was finally able to reproduce this in the integration tests. please retry.

Member

forki commented Nov 16, 2015

so seems I was finally able to reproduce this in the integration tests. please retry.

@Rcomian

This comment has been minimized.

Show comment
Hide comment
@Rcomian

Rcomian Nov 16, 2015

That's the one. Tried that and it works fine. Much cleaner and correct set of changes:

AFTER_2.25.4.0.NE.SequenceManager.vbproj.txt

Thank you very much for your work!

Rcomian commented Nov 16, 2015

That's the one. Tried that and it works fine. Much cleaner and correct set of changes:

AFTER_2.25.4.0.NE.SequenceManager.vbproj.txt

Thank you very much for your work!

@Rcomian Rcomian closed this Nov 16, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 16, 2015

Member

Cool. Thanks for caring and reporting.

Member

forki commented Nov 16, 2015

Cool. Thanks for caring and reporting.

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