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

feat: update bootstrap_dns_android #966

Merged
merged 29 commits into from
May 4, 2021

Conversation

rurirei
Copy link
Contributor

@rurirei rurirei commented May 4, 2021

this makes:

  1. the bootstrap dialer on both network tcp or udp
  2. the public number for alternative dialer

@rurirei rurirei marked this pull request as draft May 4, 2021 03:08
@rurirei rurirei marked this pull request as ready for review May 4, 2021 03:53
@rurirei rurirei changed the title feat: update bootstrapdns with network feat: update bootstrap_dns_android May 4, 2021
@rurirei
Copy link
Contributor Author

rurirei commented May 4, 2021

@CalmLong opinion wanted.

@CalmLong
Copy link
Contributor

CalmLong commented May 4, 2021

是需要通过外部来更改 bootstrapDNS 吗

@rurirei
Copy link
Contributor Author

rurirei commented May 4, 2021

@CalmLong thanks for approving. you can find a example in _test.go

@CalmLong
Copy link
Contributor

CalmLong commented May 4, 2021

只是这里是替换了 net.DefaultResolver,UseAlternativeBootstrapDNS 改为 UseCustomBootstrapDNS 会不会好一些

@rurirei
Copy link
Contributor Author

rurirei commented May 4, 2021

@CalmLong the naming is referenced internet.UseAlternativeSystemDialer in system_dialer.go

@CalmLong
Copy link
Contributor

CalmLong commented May 4, 2021

ok

@rurirei rurirei marked this pull request as draft May 4, 2021 09:02
@xiaokangwang
Copy link
Contributor

You can tell me in detail the root reason you are attempting this change.

What kind of environment you are using this in?

Are you using V2Ray as standalone program in Android, or as a library?

If you are using it as a standalone program, please provide full configure file.
If you are using it as a library, please provide a link of code.

What is the thing you are expecting, and what unexpected thing happened?

@rurirei
Copy link
Contributor Author

rurirei commented May 4, 2021

a revert is done here.

@rurirei rurirei changed the title feat&refine: systemDNS for systemDialer feat: update bootstrap_dns_android May 4, 2021
@rurirei rurirei marked this pull request as ready for review May 4, 2021 14:28
Copy link
Contributor

@xiaokangwang xiaokangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK,non-android platforms shouldn't be influence by this change now. I will merge this before next release.

@xiaokangwang
Copy link
Contributor

a revert is done here.

You can tell me in detail the root reason you are attempting this change.

What kind of environment you are using this in?

Are you using V2Ray as standalone program in Android, or as a library?

If you are using it as a standalone program, please provide full configure file.
If you are using it as a library, please provide a link of code.

What is the thing you are expecting, and what unexpected thing happened?

@xiaokangwang
Copy link
Contributor

I wants to know the exact reason you wants to make this change so that I can have a long term solution for you.

@rurirei
Copy link
Contributor Author

rurirei commented May 4, 2021

I wants to know the exact reason you wants to make this change so that I can have a long term solution for you.

#966 (comment)

@xiaokangwang
Copy link
Contributor

I wants to know the exact reason you wants to make this change so that I can have a long term solution for you.

#966 (comment)

This is what you have changed, not why you have changed it.

If you are using it as a standalone program, please provide full configure file.
If you are using it as a library, please provide a link of code.

What is your exact use case?

@xiaokangwang
Copy link
Contributor

I wants to find a permanent solution to your problem, and add it to the to do list

@rurirei
Copy link
Contributor Author

rurirei commented May 4, 2021

@xiaokangwang

  1. as library (AndroidLibV2ray based)
  2. i need to override the dns address "8.8.8.8:53", and make a custom dns resolver
  3. i want to a method to make the custom dialer.Resolver without uses internet.UseAlternativeSystemDialer

@xiaokangwang
Copy link
Contributor

@xiaokangwang

1. as library (AndroidLibV2ray based)

2. i need to override the dns address "8.8.8.8:53"

OK, I will work toward a permanent solution to your issue. Can you give me a link of your client. This can help me assign priority.

@rurirei
Copy link
Contributor Author

rurirei commented May 4, 2021

@xiaokangwang in fact internet.UseAlternativeSystemDialer() is not necessary for AndroidLibV2ray (method fdConn in AndroidLibV2ray is not necessary for work). i need a method to make a custom systemDialer.Resolver only, rather than to override the systemDialer (what method fdConn do). the reverted "systemDNS for systemDialer" commits is what i really wanted.

you can kindly do solve the problem exists pastly. thanks.

@rurirei rurirei marked this pull request as draft May 4, 2021 15:11
@xiaokangwang
Copy link
Contributor

xiaokangwang commented May 4, 2021

@xiaokangwang in fact internet.UseAlternativeSystemDialer() is not necessary for AndroidLibV2ray (method fdConn in AndroidLibV2ray is not necessary for work). i need a method to make a custom systemDialer.Resolver only, rather than to override the systemDialer (what method fdConn do). the reverted "systemDNS for systemDialer" commits is what i really wanted.

you can kindly do solve the problem exists pastly. thanks.

You don't have to convert this to a draft.

I will merge this PR as a temporary solution. You can use this temporary solution today while I am working on a permanent one.

Please mark this PR as ready for review so that I can merge it.

@rurirei rurirei marked this pull request as ready for review May 4, 2021 15:18
@xiaokangwang
Copy link
Contributor

xiaokangwang commented May 4, 2021

Ok, I will merge this before the next release.

@xiaokangwang xiaokangwang merged commit 2129c6e into v2fly:master May 4, 2021
@xiaokangwang
Copy link
Contributor

@rurirei I am sorry to inform you that because of some of the recent PR you have made to this repo. ( #852 #966 #852 #831 ) I have requested team member to consider your PR ineligible for merge by organization members. A review from the organization owner will be required to merge your further PR. This status may be waivered later.

You have repeatably sent Pull Request to upstream to get your code pass compilation without understanding the implication and consequence of such change.

I am committed to helping you with achieving what you want to do without compromising the code quality, standard, and objective of V2Ray.

@rurirei
Copy link
Contributor Author

rurirei commented May 5, 2021

@xiaokangwang i'm sorry for bad codes.

@xiaokangwang
Copy link
Contributor

xiaokangwang commented May 5, 2021

@xiaokangwang i'm sorry for bad codes.

Don't worry, we have all been inexperienced at some point, and I have made a lot of mistakes in the past(and now) as well. It is not your fault. I just wants to reduce the revert action whenever possible.

But, I am here to help you! I can discuss your change with you and get things working together. Just make sure don't make any major change before asking, you might waste your time and I will feel really bad to request you to redo the work.

@rurirei
Copy link
Contributor Author

rurirei commented May 5, 2021

I just wants to reduce the revert action whenever possible.

i'm glad to see your help of course.

p.s. what i did is just pull request, not commiting? 😢

@xiaokangwang
Copy link
Contributor

Yes, but if someone else merged your code without the necessary review, I will have to revert it at a later point.

@rurirei
Copy link
Contributor Author

rurirei commented May 5, 2021

got it. draft is the good idea.

@xiaokangwang
Copy link
Contributor

And, you can request for review at any point. ^_^ I am ready when you are.

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.

3 participants