Skip to content

Conversation

@baronfel
Copy link
Member

@baronfel baronfel commented May 16, 2022

Fixes #25450 by

  • adding a test that covers the promotion of the preview term to a target TFM
  • adding tests that verify that for all of the known AnalysisLevel terms, we resolve those terms to an actual version number before moving on

Potential enhancements I'd like some feedback on:

  • right now I'm hard-coding the 3 known string AnalysisLevel values. Is there an enum of these available somewhere to reference? Guessing not, since this is entirely an MSBuild-level construct. Resolution: just hard-coded them for now.

  • the 'preview floats to the next TFM' test currently hard-codes 8.0. I would love to be able to get the TFM we're built for at runtime somehow. anyone know how to do this? maybe a custom AssemblyAttribute? Resolution: Used the ToolSetInfo to ensure that when we bump our concept of 'next' and 'current' TFMs the sentinel test will break, so we will have to update this implementation.

  • currently it's hard to tell the user the valid values of the AnalysisLevel property via MSBuild warnings or errors, because all of this calculation happens as part of property evaluation. Other .targets files do this property evaluation inside of a target to allow for creating NETSDK errors. Is it worth trying to move in that direction? What I'm trying to fix is unknown AnalysisLevels getting passed into a TFM version evaluation because of this condition:

        <EffectiveAnalysisLevel Condition="'$(EffectiveAnalysisLevel)' == '' And
                                         '$(AnalysisLevel)' != ''">$(AnalysisLevel)</EffectiveAnalysisLevel>

    Resolution: too hard and error prone to do - will discuss with SDK/MSBuild team for a later date

@ghost ghost added the Area-NetSDK label May 16, 2022
@baronfel baronfel requested a review from jmarolf May 16, 2022 15:52
@jmarolf
Copy link
Contributor

jmarolf commented May 17, 2022

right now I'm hard-coding the 3 known string AnalysisLevel values. Is there an enum of these available somewhere to reference? Guessing not, since this is entirely an MSBuild-level construct.

There is no better way today as far as I know. We could have an enum and then generate the MSBuild definitions from there.

@jmarolf
Copy link
Contributor

jmarolf commented May 17, 2022

Other .targets files do this property evaluation inside of a target to allow for creating NETSDK errors. Is it worth trying to move in that direction?

I think this is a good idea. If the developer provides an invalid value we should error.

@baronfel baronfel force-pushed the resolve-analysislevel-terms branch from 7fb7a22 to 679f43b Compare May 18, 2022 15:08
@baronfel
Copy link
Member Author

baronfel commented Jun 1, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@baronfel
Copy link
Member Author

baronfel commented Jun 2, 2022

Merging since these are test additions only.

@baronfel baronfel merged commit e6f06f6 into dotnet:main Jun 2, 2022
@baronfel baronfel deleted the resolve-analysislevel-terms branch June 2, 2022 14:09
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.

AnalysisLevel preview setting is missing tests

2 participants