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

Compatibility change for go 1.18 #2317

Merged
merged 2 commits into from Feb 18, 2023
Merged

Compatibility change for go 1.18 #2317

merged 2 commits into from Feb 18, 2023

Conversation

MoetaYuko
Copy link
Contributor

No description provided.

@AkinoKaede
Copy link
Contributor

The version in go.mod should be change to 1.18. However, is it necessary to maintain compatibility with Go 1.18?

@MoetaYuko
Copy link
Contributor Author

The version in go.mod should be change to 1.18.

It was changed to 1.19 in 9ae51c1, and at that time 1.18 was still supported. I'm unsure how to deal with it but should be irrelevant to this PR IMHO.

However, is it necessary to maintain compatibility with Go 1.18?

The reason is two-fold:

  1. OpenWRT 21.02 (a.k.a. "oldstable") uses go 1.18 and it's unlikely that the branch will receive new updates of go.
  2. The extra effort to support 1.18 is really trivial for now at least.

@AkinoKaede
Copy link
Contributor

The version in go.mod specifies the minimum version to use. For normative reasons, this field should be changed if support for Go 1.18 is to be maintained.

The go directive in a go.mod file now indicates the version of the language used by the files within that module. It will be set to the current release (go 1.12) if no existing version is present. If the go directive for a module specifies a version newer than the toolchain in use, the go command will attempt to build the packages regardless, and will note the mismatch only if that build fails. Go 1.12 Release Notes

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Base: 39.19% // Head: 39.16% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (4d4024c) compared to base (1aac75c).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2317      +/-   ##
==========================================
- Coverage   39.19%   39.16%   -0.03%     
==========================================
  Files         617      617              
  Lines       36859    36859              
==========================================
- Hits        14448    14437      -11     
- Misses      20821    20828       +7     
- Partials     1590     1594       +4     
Impacted Files Coverage Δ
app/proxyman/outbound/errors.generated.go 0.00% <0.00%> (-100.00%) ⬇️
common/task/task.go 88.88% <0.00%> (-7.41%) ⬇️
app/proxyman/outbound/handler.go 35.61% <0.00%> (-4.11%) ⬇️
common/session/context.go 56.66% <0.00%> (-1.67%) ⬇️
transport/internet/kcp/receiving.go 97.31% <0.00%> (-1.35%) ⬇️
proxy/vmess/outbound/outbound.go 58.26% <0.00%> (-0.79%) ⬇️
common/log/logger.go 82.35% <0.00%> (+1.47%) ⬆️
transport/internet/quic/hub.go 74.66% <0.00%> (+2.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dyhkwong
Copy link
Contributor

qtls.CipherSuiteTLS13 and qtls.AEADAESGCMTLS13 in different qtls versions are just the same thing. It is possible to embed them into v2ray to remove those annoying qtls-go1-xx dependencies.

@MoetaYuko
Copy link
Contributor Author

qtls.CipherSuiteTLS13 and qtls.AEADAESGCMTLS13 in different qtls versions are just the same thing. It is possible to embed them into v2ray to remove those annoying qtls-go1-xx dependencies.

This is how Xray deal with the issue XTLS/Xray-core@00c9576, but I'm wondering if it'll silently break stuff if qtls changes them in the future.

I'm personally open to both solutions.

@MoetaYuko
Copy link
Contributor Author

@AkinoKaede Any updates? Which solution is preferred?

@AkinoKaede
Copy link
Contributor

AkinoKaede commented Feb 13, 2023

My concern is that V2Ray may not support Go 1.18 in the long term. quic-go will stop supporting Go 1.18 when Go 1.21 is released, as a rule.

@AkinoKaede
Copy link
Contributor

c.c. @xiaokangwang

@xiaokangwang
Copy link
Contributor

I agree that QUIC is problematic, but I think we could not keep the compatibility with a certain version of Go tool chain indefinitely.

Is there anything preventing this change from being send to OpenWRT's distro repo, which is prefer for this kind of distro specific changes?

@xiaokangwang xiaokangwang added the Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval label Feb 16, 2023
@MoetaYuko
Copy link
Contributor Author

I agree that QUIC is problematic, but I think we could not keep the compatibility with a certain version of Go tool chain indefinitely.

Of course, it's infeasible to keep compatibility with go 1.18 forever. However, I think it's fair to add the compatibility for now at least considering the trivial extra effort. Besides, v2ray itself is buildable with go 1.18 but a dependence (QUIC). It's more reasonable to drop support for old go releases when v2ray strictly relies on features from new go releases.

Is there anything preventing this change from being send to OpenWRT's distro repo, which is prefer for this kind of distro specific changes?

Yes and no. For building v2ray itself we can simply patch it. But for packages using v2ray as a go dependence (e.g. https://github.com/teddysun/v2ray-plugin/blob/b06bccfb77fd7f1fad4769302e47967ad7dbd8d7/go.mod#L7), I'm not aware of any built-in mechanism to patch it.

@xiaokangwang
Copy link
Contributor

I think it is fine to merge this and buy OpenWRT more time to deal with their slow update issue and make things easier for you. It should be remembered that this is a temporary measure, and will not be kept indefinitely.

I don't think it is possible to get rid of QUIC now with selective compile since it is imported by app/dns, so we will have to drop support for go 1.18 when QUIC stop support it.

@MoetaYuko
Copy link
Contributor Author

Rebased just now to resolve conflicts.

I think it is fine to merge this and buy OpenWRT more time to deal with their slow update issue and make things easier for you. It should be remembered that this is a temporary measure, and will not be kept indefinitely.

22.03 and upwards have migrated to newer golang releases so ig 21.02 will stay 1.18 forever. Nonetheless, support for 21.02 will be dropped soon after 23.xx release this year, so this "temporary" change should be sufficient before 21.02 eol.

@xiaokangwang xiaokangwang merged commit 0dcf8ca into v2fly:master Feb 18, 2023
@MoetaYuko
Copy link
Contributor Author

quic-go 0.33.0 is no longer buildable with go 1.18, so this becomes a no-op now and can be reverted.

It's happening so faaaast

@AkinoKaede
Copy link
Contributor

:(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extensive Review Required Require an extensive review from organization owner, cannot be merged without owner approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants