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

Select() is not supported when app configured with UWP Sideload #262

Closed
0x5bfa opened this issue May 30, 2022 · 10 comments
Closed

Select() is not supported when app configured with UWP Sideload #262

0x5bfa opened this issue May 30, 2022 · 10 comments

Comments

@0x5bfa
Copy link
Contributor

0x5bfa commented May 30, 2022

When configured with Debug, app will process successfully, but with Sideload, will fail.

Is there the solution to it?

2022-05-31 00:09:44.870 +09:00 [ERR] LoadHomeContentsAsync
System.NotSupportedException: Select() is not supported
   at Octokit.GraphQL.Core.Builders.QueryBuilder.RewriteValueExtension(MethodCallExpression, MemberInfo) + 0x6b8
   at Octokit.GraphQL.Core.Builders.QueryBuilder.VisitMethodCall(MethodCallExpression, MemberInfo) + 0x56
   at Octokit.GraphQL.Core.Builders.QueryBuilder.VisitMethodCall(MethodCallExpression) + 0xb
   at System.Linq.Expressions.MethodCallExpression.Accept(ExpressionVisitor) + 0x16
   at System.Linq.Expressions.ExpressionVisitor.Visit(Expression) + 0x18
   at Octokit.GraphQL.Core.Builders.QueryBuilder.Build[TResult](IQueryableValue`1) + 0x40
   at FluentHub.Octokit.Queries.Users.UserQueries.<GetViewerLogin>d__1.MoveNext() + 0x230
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() + 0x21
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task) + 0x70
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task) + 0x38
   at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task) + 0x17
   at FluentHub.ViewModels.Home.UserHomeViewModel.<LoadHomeContentsAsync>d__14.MoveNext() + 0x19f

FluentHub.Octokit is .NET Standard project. but solution configuration is UWP app.

@0x5bfa 0x5bfa changed the title Select() is not supported when app configured with Sideload Select() is not supported when app configured with UWP Sideload May 30, 2022
@eerhardt
Copy link

I ran into this same problem trying to use Octokit.GraphQL in a .NET NativeAOT application.

The issue was spotted by @MichalStrehovsky. Octokit.GraphQL targets netstandard1.1 and has the following code:

if (expression.Method.GetGenericMethodDefinition() == QueryableValueExtensions.SelectMethod)
{
var source = expression.Arguments[0];
var selectExpression = expression.Arguments[1];
var lambda = selectExpression.GetLambda();
var instance = Visit(AliasedExpression.WrapIfNeeded(source, alias));
var select = (LambdaExpression)Visit(lambda);
var selectMethod = select.ReturnType == typeof(JToken) ?
Rewritten.Value.SelectJTokenMethod :
Rewritten.Value.SelectMethod.MakeGenericMethod(select.ReturnType);
return Expression.Call(
selectMethod,
instance,
select);
}
else if (expression.Method.GetGenericMethodDefinition() == QueryableValueExtensions.SelectFragmentMethod)
{

The problem is that netstandard1.1 doesn't define the == operator for MethodInfo/MethodBase. You can see this at https://apisof.net/catalog/2944695d-6736-56dc-e4d6-1328ed10ac7e and https://apisof.net/catalog/8358cb4a-bbf2-27c0-6a7e-f419f599caf6.

image

Note there is no ".NET Standard 1.x" entry on the right side.

So the == operator is falling back to object.==, which means "Reference Equals". In "normal" .NET applications, MethodInfos are cached, and so this typically works just fine. However for UWP and NativeAOT, the MethodInfos aren't cached and so there are multiple instances of MethodInfo for the same logical method.

This can be fixed 2 ways for NativeAOT (and I assume the same fixes will work for UWP):

  1. Change all the methodInfo1 == methodInfo2 calls to be methodInfo1.Equals(methodInfo2).
    a. The downside of this approach is that it is really easy for more code to be added to use the == operator for MethodInfos and would reintroduce the same issue.
  2. Change the TFM that Octokit.GraphQL targets from netstandard1.1 to netstandard2.0.
    a. Alternatively, the library target both netstandard1.1 and netstandard2.0, which would fix the problem as well. However,
    b. The .NET team recommends to just target netstandard2.0 and avoid targeting netstandard1.x unless you absolutely must. All modern/supported versions of .NET support netstandard2.0, and so there is very little reason to target netstandard1.x anymore. Doing it can actually cause problems. See https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting for more information.

I can make a PR for either of the above approaches. I'm just not sure which way the owners would want to go here. My recommendation is to change netstandard1.1 to netstandard2.0.

cc @jeffhandley @nickfloyd

@0x5bfa
Copy link
Contributor Author

0x5bfa commented Jul 14, 2022

Thank you for responding and solution to this issue!!

We've just updated .NET platform to netstandard2.0 in the PR. We will run RELEASE mode app package after merged this PR and then tell you whether it succeeded or not!

@eerhardt
Copy link

We've just updated .NET platform to netstandard2.0 in the 0x5bfa/FluentHub#197.

The fix I'm describing above is that Octokit.GraphQL itself needs to target netstandard2.0. Changing these lines here:

<TargetFramework>netstandard1.1</TargetFramework>

<TargetFramework>netstandard1.1</TargetFramework>

@0x5bfa
Copy link
Contributor Author

0x5bfa commented Jul 14, 2022

the owners

Who is the owner? @eerhardt

@Lamparter
Copy link
Contributor

Who is the owner?

@jeffhandley @nickfloyd
@onein528 these are the owners

@eerhardt
Copy link

Who is the owner?

From https://www.nuget.org/packages/Octokit.GraphQL/:

image

@jeffhandley

@jeffhandley is on the .NET team with me. I cc'd him because he is working on making a GitHub Action that uses .NET NativeAOT and ran into this problem with me.

@nickfloyd
Copy link
Contributor

Hey friends, we just made this update in octokit.net and Octokit Webhooks.net. I planned to get to this today unless someone else would like to pick it up. The plan is to convert everything to .NET Standard 2.0. Let me know if anyone has other thoughts or feelings on that move.

@nickfloyd
Copy link
Contributor

oof. I knew this sounded familiar 😬 . We (specifically @onein528 - he mentioned this issue in #270 several days ago) took care of this in #270.

This change is in this release and is out on nuget

Apologies for the confusion - we've been updating Octrokit.net libs to Standard 2.0 so I got a bit foggy on this one.

Let me know if everyone agrees that this is good to close.

@eerhardt
Copy link

Let me know if everyone agrees that this is good to close.

Yes, I agree this issue can now be closed as resolved by #270.

@0x5bfa
Copy link
Contributor Author

0x5bfa commented Aug 12, 2022

So do I. Thank you @nickfloyd !

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

No branches or pull requests

4 participants