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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if references file exists on disk before installing into project file #2224

Merged
merged 2 commits into from Apr 10, 2017

Conversation

Projects
None yet
3 participants
@bchavez
Contributor

bchavez commented Apr 9, 2017

Check if paket.references file exists on disk before allowing paket to blindly install/modify (csproj) project files with <Import>. If paket.references is missing from a project directory, paket should not touch any project files.

Also, according to @forki:

If you remove the paket.references file from the dotnetcore project folder
then it should ignore the csproj.

This is currently not true as there is no check here to exclude project files (csprojs) from the FindAllProjects enumeration. Additionally, this particular issue has caused some drama in issue #2029.

This PR resolves the issue and makes sure that paket.references exists on disk before paket does anything to MSBuild project files.

Also resolves #2220 by providing a way to opt-out of the <Import> mechanism.

Thanks,
Brian

馃崼 馃崻 馃嵀 Ronald Jenkees - Stay Crunchy

@bchavez

This comment has been minimized.

Show comment
Hide comment
@bchavez

bchavez Apr 9, 2017

Contributor

And, in addition, got your Travis CI build working again. 馃憤

馃毝 馃槑 "So don't delay... act now supplies are running out..."

Contributor

bchavez commented Apr 9, 2017

And, in addition, got your Travis CI build working again. 馃憤

馃毝 馃槑 "So don't delay... act now supplies are running out..."

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Apr 10, 2017

Member

@agross do you remember how we defined that case?

If a csproj doesn't have a corresponding references file - should treat it as empy or do we ignore the project?

Member

forki commented Apr 10, 2017

@agross do you remember how we defined that case?

If a csproj doesn't have a corresponding references file - should treat it as empy or do we ignore the project?

@agross

This comment has been minimized.

Show comment
Hide comment
@agross

agross Apr 10, 2017

Contributor

@forki If a ??proj doesn't have a paket.references or the specific proj.??proj.references (not sure about the naming convention right now) file next to it, how would paket decide what to install in the first place. So I think yes, we need to ignore it.

Contributor

agross commented Apr 10, 2017

@forki If a ??proj doesn't have a paket.references or the specific proj.??proj.references (not sure about the naming convention right now) file next to it, how would paket decide what to install in the first place. So I think yes, we need to ignore it.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Apr 10, 2017

Member

how would paket decide what to install in the first place

currently it's creating an empty references file in memory. So it does not install anything - but it touches it since it wants to clean.

Member

forki commented Apr 10, 2017

how would paket decide what to install in the first place

currently it's creating an empty references file in memory. So it does not install anything - but it touches it since it wants to clean.

@bchavez

This comment has been minimized.

Show comment
Hide comment
@bchavez

bchavez Apr 10, 2017

Contributor

So it does not install anything - but it touches it since it wants to clean.

Yes it does install something in the .??proj file, this right here:

<Import Project="..\.paket\Paket.Restore.targets" />

Even when there is no paket.references. This is the problem in #2029 and #2220 鉂楋笍鉂楋笍鉂楋笍

Contributor

bchavez commented Apr 10, 2017

So it does not install anything - but it touches it since it wants to clean.

Yes it does install something in the .??proj file, this right here:

<Import Project="..\.paket\Paket.Restore.targets" />

Even when there is no paket.references. This is the problem in #2029 and #2220 鉂楋笍鉂楋笍鉂楋笍

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Apr 10, 2017

Member

I wished you would not see there targets as a problem. It's the only solution to get netcore working. They might not work for you yet. but it's the important part that needs fixing.

Member

forki commented Apr 10, 2017

I wished you would not see there targets as a problem. It's the only solution to get netcore working. They might not work for you yet. but it's the important part that needs fixing.

@forki forki merged commit 374e321 into fsprojects:master Apr 10, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

bchavez added a commit to bchavez/Bogus that referenced this pull request Apr 10, 2017

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