Skip to content
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

Set DeployExtension=false in BuildAndTest.proj... #6574

Merged
merged 1 commit into from
Nov 7, 2015

Conversation

KevinH-MS
Copy link
Contributor

BuildAndTest.cmd already sets this property. We should have the same
behavior if you're performing the analogous operation in the "Open"
submodule. Since we never run IDE tests as a part of BuildAndTest, it
shouldn't matter that we don't deploy the extensions. Without this,
"msbuild BuildAndTest.proj" doesn't work on clean VS install.

BuildAndTest.cmd already sets this property.  We should have the same
behavior if you're performing the analogous operation in the "Open"
submodule.  Since we never run IDE tests as a part of BuildAndTest, it
shouldn't matter that we don't deploy the extensions.  Without this,
"msbuild BuildAndTest.proj" doesn't work on clean VS install.
@KevinH-MS
Copy link
Contributor Author

FYI @dotnet/roslyn-infrastructure
/cc @TyOverby

@KevinH-MS
Copy link
Contributor Author

I think it's best to just handle this in the "BuildAndTest" scripts for now (rather than pushing the logic deeper into our build .targets, because that would involve checking to see if the VS extensions folder contained Roslyn...presumably that logic would all have to change or go away once VsSdk starts preferring user local extensions over the ones installed in the VS extensions folder).

@TyOverby
Copy link
Contributor

TyOverby commented Nov 5, 2015

👍

@jaredpar
Copy link
Member

jaredpar commented Nov 5, 2015

CC @jasonmalinowski

Does setting the property in this way override properties that are explicitly set in the csproj / vbproj files? The previous solution of specifying on the command line does, but I'm unsure if this method will. If it doesn't then we need to modify the use of <DeployExtension>true</DeployExtension> in our code base.

@jasonmalinowski
Copy link
Member

So there are two answers here:

  1. This will override how you think it will.
  2. This isn't what you want to do. I would expect as a end user calling BuildAndTest that it deploys my extensions to the hive. I 100% agree that we don't want to deploy on cibuild, but I would suspect you do want to isolate that decision into the cibuild scripts.

@jasonmalinowski
Copy link
Member

And I guess following up to #2: I can totally imagine if you're turning this off real quick until we get Update 1 out and then we'll fix this all up. But this isn't what we want, Eventually™.

@KevinH-MS
Copy link
Contributor Author

Bottom line is that, today, if you just install VS and don't uninstall the Roslyn msi, then "msbuild BuildAndTest.proj" generates errors. These errors are not at all informative to the uninitiated developer.

I would propose that we do this until we're able to fix this up on top of Update 1 (unless anyone is really relying on "msbuild BuildAndTest.proj" deploying their extensions...which I doubt...).

I assert that "everyone" is using "msbuild Roslyn.sln" to deploy extensions for testing.

@KevinH-MS
Copy link
Contributor Author

Any strong objections? Going once...going twice...

KevinH-MS added a commit that referenced this pull request Nov 7, 2015
Set DeployExtension=false in BuildAndTest.proj...
@KevinH-MS KevinH-MS merged commit 24004e8 into dotnet:master Nov 7, 2015
KevinH-MS added a commit to KevinH-MS/roslyn that referenced this pull request Dec 15, 2015
This reverts commit 24004e8, reversing
changes made to 0cef61e.
KevinH-MS added a commit that referenced this pull request Dec 17, 2015
Revert "Merge pull request #6574 from KevinH-MS/master"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants