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

C# signatures for new operators #713

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nickwalkmsft
Copy link
Collaborator

Work items:

Implement correct C# method signature output for the newer operators that currently aren't implemented: unsigned right shift and all of the checked operators.

Added unit tests and tested locally against the types mentioned in the work items (BigInteger and Int128).

As with #710, I was unable to get C++ tests running, but nothing in that codepath should be affected.

Implement correct C# method signature output for the newer operators
that currently aren't implemented: unsigned right shift and all of the
checked operators.
@v-fuquanli
Copy link
Collaborator

@huangmin-ms
Copy link
Collaborator

huangmin-ms commented Sep 26, 2024

https://ceapex.visualstudio.com/Engineering/_build/results?buildId=1868434

@nickwalkmsft The automatic trigger stops working somehow. As a workaround, you can run it manually in https://ceapex.visualstudio.com/Engineering/_build/results?buildId=1868434&view=results and enter your branch name.
image
Please make sure the pipeline is passed.

@nickwalkmsft
Copy link
Collaborator Author

nickwalkmsft commented Sep 26, 2024

@huangmin-ms bummer, both this and #712 failed on compilation errors. It looks like the build uses .NET6/C#10, but checked user-defined operators weren't added until .NET7/C#11, and ref readonly parameters in .NET8/C#12. I don't think the actual implementation changes require newer versions, but the test cases actually use the newer language features, and so they don't compile in the build.

What does it take to update the platform version used to compile and run mdoc? I imagine it has the potential to require some breakfixes, updates to the runtime environments used in the .NET pipelines that run mdoc, etc.

@huangmin-ms
Copy link
Collaborator

@nickwalkmsft

For this PR, the compile error is caused by the mcs (mono) command.

mcs -target:library -unsafe -debug -optimize -target:library -out:Test/TestClass.dll mdoc.Test/SampleClasses/Test*.cs

I downloaded the latest mono and be able to reproduce this issue in local. To get rid of this in the future, we might need to replace all mcs command with dotnet build but that requires additional effort.

For #712, I upgraded pipeline sdk to 6.x to 8.x and the compilation error is gone. But seems the build result is also changed and we need to fix that. https://ceapex.visualstudio.com/Engineering/_build/results?buildId=1880166&view=logs&j=15dfcb1a-0989-5cf6-3160-3e181e44de87&t=ee259841-8374-5502-dbdb-28d76f0fa540

@nickwalkmsft
Copy link
Collaborator Author

Thanks for digging on this @huangmin-ms! I admittedly didn't spend much time looking at the build, I just set up a mostly-working dev environment using VS2022 locally and worked on the code from there, so it doesn't surprise me that there are build-related problems.

There's no hurry for these items, they can remain at their current priority, so please don't feel there's any rush to resolve the issues. Thanks!

@xmdanni
Copy link
Collaborator

xmdanni commented Nov 15, 2024

Update: the automatic trigger for PR stops working because someone enforced below rule in the org/project where the pipeline is in, and they did not notify us. With this enforcement, it is required to add a comment /azp run in the PR to trigger the pipeline. I can try some workaround later if needed. @nickwalkmsft @huangmin-ms @v-fuquanli
image

@xmdanni
Copy link
Collaborator

xmdanni commented Nov 15, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@xmdanni
Copy link
Collaborator

xmdanni commented Nov 15, 2024

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

Successfully merging this pull request may close these issues.

4 participants