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

Setting CSharpProjectConfigurationProperties3.LanguageVersion creates verbose XML in project file #2732

Closed
jcouv opened this issue Aug 25, 2017 · 23 comments · Fixed by #3547
Closed
Labels
Feature-Project-File-Simplification Related to cleaning up and simplification of the project format. Legacy Issues against the legacy project system. Triage-Approved Reviewed and prioritized
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Aug 25, 2017

The UpgradeProject code fixer produces a lot of "fluff" in the project file:

<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|AnyCPU'">
  <LangVersion>latest</LangVersion>
</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'">
  <LangVersion>latest</LangVersion>
</PropertyGroup>

I would expect a single <LangVersion>latest</LangVersion> to be added to the standard, non-conditional, <PropertyGroup/> instead.

Thanks @DavidArno for reporting this. Also FYI @jnm2 @BillWagner

@davkean
Copy link
Member

davkean commented Aug 25, 2017

There's really two bugs; one for the legacy and one for new project system.

New project system, let's just make langversion a non-config property. Easy peasy.
Legacy project system, a big more work and compat to think about. You're going through a "configuration" so the behavior you are seeing is definitely expected. We'd need to introduce a dual concept here where reading would read via configuration (for compat) but write would bypass config and set it project-wide.

@davkean
Copy link
Member

davkean commented Aug 25, 2017

The setting is also on a config-based page so we'd need to think about that as well./

@jnm2
Copy link

jnm2 commented Aug 25, 2017

I wish the build properties page opened to show all configurations by default, and thus any change made would first remove all conditional <LangVersion> elements from the .csproj and then add/update an unconditional <LangVersion> element.

@davkean
Copy link
Member

davkean commented Aug 25, 2017

That's an interesting idea - perhaps we have a way of setting "all configs" and just not condition the property when you set that.

@jnm2
Copy link

jnm2 commented Aug 25, 2017

The same would be very helpful for <AllowUnsafeBlocks>, <WarningsAsErrors />, <TreatWarningsAsErrors>. and <DocumentationFile>. It only results in confusion when any of these properties has a configuration-conditional value- the feedback loop for discovering errors is much longer, when you push to CI for example.

@DavidArno
Copy link

@davkean,

My original request was specifically related to the new csproj file format. You guys did such an amazing job with that new format that I've happily spent time rewriting all old csproj files in my code to the new format. So - personally - I don't much care what happens with legacy projects 😉

(I appreciate you have to look at a much bigger picture than me though and so will need to address both scenarios)

@yaakov-h
Copy link
Member

Even if I select 'All Configurations' before changing the language version, I get the conditional duplication defined in the OP (new project system, haven't tried old).

Shouldn't this define it once for all configurations, rather than N times for each current configuration?

@davkean
Copy link
Member

davkean commented Aug 25, 2017

Yes that's one of the proposal aboves. Legacy project system has always had this behavior - and the new project system kinda mirrored it. We should fix that.

@jnm2
Copy link

jnm2 commented Aug 25, 2017

So - personally - I don't much care what happens with legacy projects

Gotta say I'd prefer the energy be put into allowing winforms projects to use the new sdk so I can just get off the legacy sdk altogether :D

@jcouv
Copy link
Member Author

jcouv commented Mar 26, 2018

Note: a number of users reported pain with this setting being per-configuration (see comments in dotnet/roslyn#21783 (comment)).

@DavidArno
Copy link

Will this likely be fixed any time soon? As @davkean says, it ought to be easy to fix for the new project file format, but six months later it's still doing it the messy way.

@davkean
Copy link
Member

davkean commented Mar 27, 2018

@DavidArno We're focusing on bugs that affect inner loop and those without workarounds. While annoying, this is a once per project change that has an easy workaround to simplify resulting comparison.

@Neme12
Copy link

Neme12 commented Apr 14, 2018

@davkean Aren't all configuration-specific options also possible to be set independently of configuration? If that's the case, the ConfigurationManager API could have a special property (maybe AllConfigurations) that would return a fake Configuration that would always write configuration-independent values.

@davkean davkean removed the Feature-Project-File-Editing Editing the project either while open in Visual Studio, or via other mechanism. label Apr 17, 2018
@Pilchie
Copy link
Member

Pilchie commented May 15, 2018

davkean added a commit to davkean/project-system that referenced this issue May 15, 2018
Fixes: dotnet#2732 for the new project system.
@davkean
Copy link
Member

davkean commented May 15, 2018

Have a fix for the new project system here: #2732 #3547. This bug will be left open to track the legacy project system.

@davkean davkean modified the milestones: 15.8, Unknown May 15, 2018
@davkean davkean added the Legacy Issues against the legacy project system. label May 15, 2018
@Neme12
Copy link

Neme12 commented May 15, 2018

Note: AllowUsafeBlocks could use this fix as well (dotnet/roslyn#25875)

@davkean
Copy link
Member

davkean commented May 15, 2018

AllowUnsafeBlocks requires a UI change - it's prominent (as opposed to langversion which is behind Advanced) on a configuration-specific page. I'd prefer tracking that as a separate bug.

@davkean davkean reopened this May 16, 2018
@davkean
Copy link
Member

davkean commented May 16, 2018

LangVersion is now configuration-agnostic for the new project system (SDK-based projects). This change will first appear in 15.8 Preview 2.

@davkean
Copy link
Member

davkean commented May 16, 2018

@Neme12 Can you please file new bugs for any other properties that you feel fall into the same bucket?

@Pilchie
Copy link
Member

Pilchie commented May 16, 2018

Correction - it will be in Preview 3 at this point.

@jjmew jjmew added Triage-Approved Reviewed and prioritized and removed Triage-Approved Reviewed and prioritized labels Jan 14, 2019
@tmeschter
Copy link
Contributor

Tom's triage notes: Recommend we just close the issue.

@tmeschter
Copy link
Contributor

We have decided to not address this in the legacy project system. Closing.

@weedkiller
Copy link

I still have this problem where even after adding latest C# compilerTools its still not using the latest C# compilers for ASP MVC 5 project on 4.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature-Project-File-Simplification Related to cleaning up and simplification of the project format. Legacy Issues against the legacy project system. Triage-Approved Reviewed and prioritized
Projects
None yet
Development

Successfully merging a pull request may close this issue.