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

Support for conditional groups #1026

Closed
matthid opened this issue Aug 28, 2015 · 12 comments
Closed

Support for conditional groups #1026

matthid opened this issue Aug 28, 2015 · 12 comments

Comments

@matthid
Copy link
Member

matthid commented Aug 28, 2015

OK I try to re-formulate the problem I mentioned in #1018 (comment).

@forki Nice, thanks!

Ok I did some more tests and was really impressed to get NO error after specifying

Microsoft.AspNet.Razor

group Net40
 Microsoft.AspNet.Razor

group Razor4
 Microsoft.AspNet.Razor

in paket.references. I think what paket does is correct even considering that the resulting >csproj will not compile.

the first two are fine because one is net45-only and the other one is net40-only. But the third >one is net45 only as well.. This means I need some way to specify which one to take at build time >(I don't want to use two references to the same nuget package on a single compilation).

One suggestion would be to allow something like

group Main if NET45
 Microsoft.AspNet.Razor

group Net40
 Microsoft.AspNet.Razor

group Razor4 if RAZOR4
 Microsoft.AspNet.Razor

in paket.reference (or even paket.dependencies, not sure right now) and then add a condition >to the msbuild references like Condition="$(RAZOR4) == 'True':

<When Condition="$(RAZOR4) == 'True' And $(TargetFrameworkIdentifier) == '.NETFramework' >And ($(TargetFrameworkVersion) == 'v4.5' Or $(TargetFrameworkVersion) == 'v4.5.1' Or >$(TargetFrameworkVersion) == 'v4.5.2' Or $(TargetFrameworkVersion) == 'v4.5.3' Or >$(TargetFrameworkVersion) == 'v4.6')">

This is a feature request, but imho one that would make the group feature more useful.

Is this a scenario you want to tackle with the new group feature?

In RazorEngine we use 3 different builds for 3 different Razor versions

  • Razor2 (last version supporting NET4.0)
  • Razor3 (current stable)
  • Razor4 (vnext, currently beta, used by some people for coreclr/ASP.net5 support)

We use conditional compilation to take care about the API differences and provide the same API on all 3 builds. The thing is that if we develop a new RazorEngine feature there is no reason to support only the latest when it will work fine on any Razor version. Handling 3 branches is not really an option either.

Now with the group feature I can compile for Razor2 and Razor3 because they target different frameworks.

Paket will generate a csproj file like this (this is simplified):

<Choose>
  <When Condition="$(TargetFrameworkVersion) == 'v4.5'">
     <ItemGroup>
      <Reference Include="System.Web.Razor">
        <HintPath>..\..\..\packages\Microsoft.AspNet.Razor\lib\net45\System.Web.Razor.dll</HintPath>
      </Reference>
    </ItemGroup>
  </When>
  <When Condition="$(TargetFrameworkVersion) == 'v4.0'">
     <ItemGroup>
      <Reference Include="System.Web.Razor">
        <HintPath>..\..\..\packages\net40\Microsoft.AspNet.Razor\lib\net45\System.Web.Razor.dll</HintPath>
        <Paket>True</Paket>
      </Reference>
    </ItemGroup>
  </When>
</Choose>

Which is perfect!

But I still can't add Razor4 to the mix, because the reference would be enabled the same time as Razor3 (when compiling for net45):

<Choose>
  <When Condition="$(TargetFrameworkVersion) == 'v4.5'">
       <ItemGroup>
        <Reference Include="Microsoft.AspNet.Razor">
          <HintPath>..\..\..\packages\razor4\Microsoft.AspNet.Razor\lib\net45\Microsoft.AspNet.Razor.dll</HintPath>
        </Reference>
      </ItemGroup>
  </When>
  <When Condition="$(TargetFrameworkVersion) == 'v4.5'">
     <ItemGroup>
      <Reference Include="System.Web.Razor">
        <HintPath>..\..\..\packages\Microsoft.AspNet.Razor\lib\net45\System.Web.Razor.dll</HintPath>
      </Reference>
    </ItemGroup>
  </When>
  <When Condition="$(TargetFrameworkVersion) == 'v4.0'">
     <ItemGroup>
      <Reference Include="System.Web.Razor">
        <HintPath>..\..\..\packages\net40\Microsoft.AspNet.Razor\lib\net45\System.Web.Razor.dll</HintPath>
      </Reference>
    </ItemGroup>
  </When>
</Choose>

Notice how TargetFrameworkVersion=v4.5 will now enable two System.Web.Razor.dll versions, which is not what I want, so I need some way to tell paket to generate an additional condition which should be true.

One way to solve this is adding a way to tell paket to add a group with a custom msbuild condition (see example syntax in from my earlier comment).

Just something to note: Groups already improve the situation as we don't need nuget.exe anymore to resolve Razor2 and Razor4 👍 . I still need to update all csproj files by hand, but at least I now have stable paths and can use paket update (of course I still need to set <paket>false</paket> for the Razor3 and Razor4 dependency).

@forki
Copy link
Member

forki commented Aug 28, 2015

Ah. You forget to specify different versions in the first dependencies file. I thought: why would someone install the same package version 3 times.

@forki
Copy link
Member

forki commented Aug 28, 2015

what if we make this feature independent from groups:

group Main 
 Microsoft.AspNet.Razor ~>1.0 condition:NET45

group Net40
 Microsoft.AspNet.Razor ~>2.0 condition:NET40

group Razor4
 Microsoft.AspNet.Razor ~> 3.0 condition:RAZOR4

@forki
Copy link
Member

forki commented Aug 28, 2015

I mean this would the be like all the settings in http://fsprojects.github.io/Paket/nuget-dependencies.html#Framework-restrictions

@matthid
Copy link
Member Author

matthid commented Aug 28, 2015

Yes that would work for me as well.
I kind of like the idea of a per-package setting.

But on the other hand I'm not sure if we need so much power and it would kind of mess with the paket idea of a "consistent view" of the dependencies.

I think the main usage should be to switch between a consistent set of dependencies instead of single packages.

@forki
Copy link
Member

forki commented Aug 28, 2015

mhm. good point.

@matthid
Copy link
Member Author

matthid commented Aug 28, 2015

How do normal settings behave on indirect references? If I have

B ~>1.0 condition:NET45
C ~>1.0

where B and C both depend on A. Then A would have the condition:NET45 setting as well, correct?
This would be a problem and a reason why condition shouldn't be a normal setting.

@forki
Copy link
Member

forki commented Aug 28, 2015

yes. that would be a conflict.

@forki
Copy link
Member

forki commented Aug 29, 2015

Ok I added (very very basic) support for this. The generated XML is probably completely wrong right now (I need real samples).

BUT: You can now specify conditions for groups and individual packages.
you can see how it works in the tests at a815b1a#diff-9fd8e28cd8dc7e84109e9d54ddbd8a84R714

Please try it out and tell me where and how it breaks ;-)

@matthid
Copy link
Member Author

matthid commented Aug 29, 2015

Everything seems to be working fine! One thing I noticed is that the condition generates the warning

MSB4130: The condition ... may have been evaluated incorrectly in an earlier version of MSBuild. Please verify that the order of the AND and OR clauses is written as intended. To avoid this warning, add parentheses to make the evaluation order explicit.

I couldn't find a lot of info about this warning, but it seems to disappear when we generate '$(RAZOR4)' == 'True' (notice the additional quotes around the variable) OR when we put the condition at the first place (instead of at the end).

@forki
Copy link
Member

forki commented Aug 29, 2015

PR against groups branch are very welcome
On Aug 29, 2015 12:04 PM, "Matthias Dittrich" notifications@github.com
wrote:

Everything seems to be working fine! One thing I noticed is that the
condition generates the warning

MSB4130: The condition ... may have been evaluated incorrectly in an
earlier version of MSBuild. Please verify that the order of the AND and OR
clauses is written as intended. To avoid this warning, add parentheses to
make the evaluation order explicit.

I couldn't find a lot of info about this warning, but it seems to
disappear when we generate '$(RAZOR4)' == 'True' (notice the additional
quotes around the variable) OR when we put the condition at the first place
(instead of at the end).


Reply to this email directly or view it on GitHub
#1026 (comment).

matthid added a commit to matthid/Paket that referenced this issue Aug 29, 2015
forki added a commit that referenced this issue Aug 31, 2015
Fix msbuild warning MSB4130, references #1026
@matthid
Copy link
Member Author

matthid commented Sep 5, 2015

Once released, RazorEngine can finally make the full switch to paket and get rid of nuget.exe 👍
Thanks @forki.

@matthid matthid closed this as completed Sep 5, 2015
@forki
Copy link
Member

forki commented Sep 5, 2015

Wow. Cool to hear.
On Sep 5, 2015 6:28 PM, "Matthias Dittrich" notifications@github.com
wrote:

Once released, RazorEngine can finally make the full switch to paket and
get rid of nuget.exe [image: 👍]
Thanks @forki https://github.com/forki.


Reply to this email directly or view it on GitHub
#1026 (comment).

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

No branches or pull requests

2 participants