You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "FlorianHockmann (via GitHub)" <gi...@apache.org> on 2024/01/31 12:16:06 UTC
[PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
FlorianHockmann opened a new pull request, #2468:
URL: https://github.com/apache/tinkerpop/pull/2468
https://issues.apache.org/jira/browse/TINKERPOP-3030
I've also noticed that the examples used camel casing for some method names which I switched to pascal casing to stay idiomatic in C#.
I had to update the Docker image to Ubuntu 20.04 because .NET 8 is not available for Ubuntu 18.04. Since 18.04 is out of support since June already, this update makes sense in general [1].
I verified that the Docker build is still working with:
```sh
./docker/build.sh --dotnet
```
VOTE +1
[1]: https://wiki.ubuntu.com/Releases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann merged PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann commented on PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468#issuecomment-1924148227
Just to clarify: This PR doesn't change which frameworks we support with Gremlin.Net. It's only about which frameworks we use to build and test it.
Gremlin.Net mainly [targets .NET Standard 2.0](https://github.com/apache/tinkerpop/blob/master/gremlin-dotnet/src/Gremlin.Net/Gremlin.Net.csproj#L21) which is implemented by various .NET frameworks like .NET 5, .NET 6, .NET 7, and .NET 8. It has .NET 6 as an additional explicit target because we optionally use some APIs which were only introduced in .NET 6 to support Websocket compression. This can therefore only be used by users on .NET 6 and higher.
Before this PR, we built and tested Gremlin.Net with .NET 6. With this PR, we're using .NET 8 instead. But users can still stay on .NET 6 (or 7 or whatever) and use newer versions of Gremlin.Net with it.
Not sure if this was clear to you and you just wanted to propose that we **test** against .NET 6 **and** .NET 8. In that case, I agree with you in general that it would be good to test on both frameworks (or better to say runtimes in this case). The problem with that however is that all contributors then also need both runtimes installed. If you just have .NET 8 installed, then the tests will fail as you also need .NET 6 in that case.
That's also why we always use the same version across all release branches.
In the end, the question comes down to how many contributors are actually affected by this and how often do we expect to find version specific bugs because of it. I guess many contributors simply don't execute .NET tests on their machines and leave that up to GH Actions or they use the Docker based setup to execute the tests. So, this probably doesn't affect many contributors.
On the other hand, [TINKERPOP-3029](https://issues.apache.org/jira/browse/TINKERPOP-3029) is also the only bug that comes to my mind which we could have catched by executing tests on different runtimes. Although I think it wouldn't have helped much as we still would have to be on .NET 8 in the first place to find it. So, the better question is how often a bug occurs that is specific to an **older** framework than the latest LTS version which we want to use by default.
I don't have a strong opinion on this. If we are confident that this doesn't really affect contributors negatively, then I'm in favor of executing the tests on multiple runtimes. Otherwise, I'd stay with our current approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "vkagamlyk (via GitHub)" <gi...@apache.org>.
vkagamlyk commented on PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468#issuecomment-1936730837
@FlorianHockmann did you leave `src\Gremlin.Net\Gremlin.Net.csproj` unchanged on purpose?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468#issuecomment-1939207359
In general I like the idea of testing both .NET 6 and .NET 8 in GHA similar to how Java is currently setup (I don't see a need for contributors to run both locally). In my mind testing both runtimes gives the strongest guarantee against issues which only appear for specific versions. .NET compatibility is not something I know very well however so I will defer to @FlorianHockmann and @vkagamlyk to determine what level of testing is required.
VOTE +0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann commented on PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468#issuecomment-1938856875
> did you leave src\Gremlin.Net\Gremlin.Net.csproj unchanged on purpose?
Yes, we still only target .NET Standard 2.0 and .NET 6. .NET 6 is there also only because we're using Websocket compression which wasn't present in .NET Standard, but was only added in .NET 6.
Users on .NET 7 or 8 can of course still use Gremlin.Net. They should get the version built for .NET 6 in that case.
We could of course add another target for .NET 7 and/or .NET 8, but I think that doesn't add any benefit if we're not using any APIs specific for these versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468#issuecomment-1921859623
I have some concerns over dropping the .NET 6 tests from 3.6-dev. .NET 6 remains supported by Microsoft for another 9 months. We should continue to support it in 3.6.x until it reaches end of life. I think it makes sense to add .NET 8 tests but it should be parallel to, not replacing the .NET 6 tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468#issuecomment-1919010560
## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2468?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`3ae5298`)](https://app.codecov.io/gh/apache/tinkerpop/commit/3ae529870bb6ad448e081fecd4ee095f667a5892?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 75.42% compared to head [(`2c0ce57`)](https://app.codecov.io/gh/apache/tinkerpop/pull/2468?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 75.15%.
> Report is 10 commits behind head on 3.6-dev.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.6-dev #2468 +/- ##
=============================================
- Coverage 75.42% 75.15% -0.27%
+ Complexity 12321 12314 -7
=============================================
Files 1032 1057 +25
Lines 59698 63470 +3772
Branches 6936 6936
=============================================
+ Hits 45029 47704 +2675
- Misses 12287 13193 +906
- Partials 2382 2573 +191
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/tinkerpop/pull/2468?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3030 Update to .NET 8 [tinkerpop]
Posted by "FlorianHockmann (via GitHub)" <gi...@apache.org>.
FlorianHockmann commented on PR #2468:
URL: https://github.com/apache/tinkerpop/pull/2468#issuecomment-1943802015
> In general I like the idea of testing both .NET 6 and .NET 8 in GHA similar to how Java is currently setup (I don't see a need for contributors to run both locally)
I agree in general. I would also like to just execute the tests in GHA for .NET 6 and 8 without requiring contributors to have both installed, but I can't find a solution for this which doesn't require ugly workarounds.
We can't just configure this for GHA unfortunately. Instead, we need to add both versions as `TargetFrameworks` to the `csproj` config files of the test projects like this to let the tests run on both versions:
```xml
<TargetFrameworks>net6.0;net8.0</TargetFrameworks>
```
But if we do that, then you also need to have both installed in order to execute the tests. If you only have .NET 8 installed, then you'll get an error:
```
Testhost process for source(s) '[...]/tinkerpop/gremlin-dotnet/test/Gremlin.Net.UnitTest/bin/Debug/net6.0/Gremlin.Net.UnitTest.dll' exited with error: You must install or update .NET to run this application.
App: [...]/tinkerpop/gremlin-dotnet/test/Gremlin.Net.UnitTest/bin/Debug/net6.0/testhost.dll
Architecture: x64
Framework: 'Microsoft.NETCore.App', version '6.0.0' (x64)
```
So, if we want to execute the tests for both versions in GHA, then we also need to test them like this locally which means that all contributors need to have both versions installed.
But since you already cast your vote and the PR has been open for 2 weeks already (longer than our 7 day cool down time), I'll wait a few more days in case any one else has any insights on this and merge the PR if not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org