Skip to content

feat(installation): support asset download via OCR_GITHUB_MIRROR - #893

Open
wu21-web wants to merge 19 commits into
alibaba:mainfrom
wu21-web:mirror
Open

feat(installation): support asset download via OCR_GITHUB_MIRROR#893
wu21-web wants to merge 19 commits into
alibaba:mainfrom
wu21-web:mirror

Conversation

@wu21-web

@wu21-web wu21-web commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Description

Chinese users without proxies configured can't download assets fast because of network restriction. Adds a OCR_GITHUB_MIRROR check and download assets from it when it is not empty or space.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)
    Tested on aliyun cloud shell:
shell@Alicloud:~$ wget "https://gh-proxy.com/raw.githubusercontent.com/wu21-web/open-code-review/refs/heads/mirror/install.sh" -O install.sh
--2026-08-13 14:18:00--  https://gh-proxy.com/raw.githubusercontent.com/wu21-web/open-code-review/refs/heads/mirror/install.sh
Resolving gh-proxy.com (gh-proxy.com)... 104.18.42.163, 172.64.147.104, 104.18.42.54, ...
Connecting to gh-proxy.com (gh-proxy.com)|104.18.42.163|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: unspecified [text/plain]
Saving to: ‘install.sh’

install.sh                                               [ <=>                                                                                                                 ]   4.03K  --.-KB/s    in 0s      

2026-08-13 14:18:02 (28.3 MB/s) - ‘install.sh’ saved [4122]

shell@Alicloud:~$ openssl dgst -sha256 install.sh 
SHA256(install.sh)= c1309cbfe422b47245bda5e963206a6bffc36d97935c5bf81d5b5a625a863dd9
shell@Alicloud:~$ export GITHUB_MIRROR_DOMAIN='github.dpik.top'
shell@Alicloud:~$ time sh install.sh 
WARNING: Using an unofficial mirror domain, which may lead to SHA256 checksum mismatches and security risks.
downloading ocr v1.9.2 (linux/amd64)...
installed ocr v1.9.2 -> /usr/local/bin/ocr

real    1m16.726s
user    0m0.493s
sys     0m0.219s
shell@Alicloud:~$ echo 'In constrast: '
In constrast: 
shell@Alicloud:~$ time curl -fsSL https://raw.githubusercontent.com/alibaba/open-code-review/main/install.sh | sh
downloading ocr v1.9.2 (linux/amd64)...
curl: (56) Unexpected EOF
error: download failed: https://github.com/alibaba/open-code-review/releases/download/v1.9.2/opencodereview-linux-amd64

real    1m33.425s
user    0m0.074s
sys     0m0.028s

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

n/a

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 2 issue(s) in this PR.

  • ✅ Successfully posted inline: 2 comment(s)

Comment thread install.ps1 Outdated
Comment thread install.sh Outdated
@wu21-web
wu21-web marked this pull request as draft August 13, 2026 14:16
@wu21-web wu21-web changed the title feat(installation): support asset download via GITHUB_MIRROR_DOMAIN_PREFIX feat(installation): support asset download via GITHUB_MIRROR_DOMAIN Aug 13, 2026
fix naming
@wu21-web
wu21-web marked this pull request as ready for review August 13, 2026 14:27
@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Nice, this is a real pain point and the change is refreshingly small. A few things I'd want sorted before this lands, roughly in order of how much they matter:

The mirror doesn't cover version resolution. When OCR_VERSION is unset — which is the path basically everyone takes with curl ... | sh — both scripts still hit api.github.com to resolve the latest tag. That's the same network you're trying to route around, so the install can still stall or die there, and failed to fetch latest release info from github api gives no hint that the mirror simply didn't apply to that request. Your own numbers hint at this: 1m16s with the mirror vs 1m33s without, and the direct run only failed at the binary step. Most of that time was probably the API call. Either send the API request through the mirror too, or at minimum document that the mirror only covers asset downloads and suggest pinning OCR_VERSION to skip resolution.

Please prefix the variable. Everything else here is OCR_INSTALL_DIR / OCR_VERSION. A bare GITHUB_MIRROR_DOMAIN is easy to inherit by accident from a shell profile or a CI image, and what it silently does is redirect a binary download to a third-party host. OCR_GITHUB_MIRROR keeps it in our namespace.

Consider fetching sha256sum.txt from GitHub directly. Right now both the binary and its checksums come from the mirror, which is exactly why you had to write that security note. But the checksum file is a few hundred bytes — even a slow direct connection can afford it. Pull it from github.com and you keep the speedup while getting the integrity guarantee back. Fall back to the mirror if you must, but warn loudly when you do.

Smaller stuff:

  • tr -d '[:space:]' strips whitespace everywhere, while the PS side only .Trim()s. So "gh proxy.com" quietly becomes ghproxy.com — a different, real domain. The two scripts should agree, and neither should silently rewrite the value.
  • No normalization: someone typing https://gh-proxy.com (natural, given the docs read like a URL) gets https://https://gh-proxy.com/... and a baffling error. Strip an optional scheme and trailing slashes.
  • The warning goes to stdout via echo — everything else in the script is printf, with errors on stderr. It also doesn't print the domain, which is the one detail a user needs if the variable arrived by accident.
  • The hardcoded /github.com/ path only works for path-proxy mirrors. Domain-substitution mirrors just 404, and the name ...DOMAIN doesn't hint at that. Worth spelling out in the docs.
  • __bold__ as a pseudo-heading isn't used anywhere else under docs/, and it sits next to **Security note:** — pick one. I'd also put the existing env-var table back before the mirror section so an optional feature isn't crowding out the basics.

Docs across all four locales is appreciated, and the security note is honest rather than hand-wavy — thanks for that. One last thing: install.ps1 looks untested. Worth running it once, even with a bogus domain, just to confirm the $($env:X.Trim()) interpolation produces the URL you expect.

@wu21-web

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Here are the answers to your questions:

  1. I didn't pass traffic from the mirror to GitHub api on purpose. As I know, most of the mirror domains won't support GitHub api. And considered that most users using ChinaNET are OK with GitHub api (tested on itdog.cn), I will skip that. And also it is hundreds of bytes (P3).
  2. Sure, no problems
  3. As you wish. I will get that fixed.
  4. Nits
    A) This is good I think, because no domain can possibly have one or more spaces.
    B) I think this is more of a documentation issue.
    C) Might carry out a new variable named OCR_MIRROR_MODE for this. But considered the majority of mirrors using xxx.xx/github.com, this will get mentioned in the documentation only.
    D) Absolutely, will fix.

@wu21-web

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Most of time there is no problem with api.github.com access in China.

Screenshot 2026-08-18 at 8 52 28 PM

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

I pulled 88bab53 and ran the shell installer on macOS arm64 to sanity-check the new paths.

No change for existing users. With OCR_GITHUB_MIRROR unset, I diffed the request sequence against main using a stub curl: same three URLs, same order, and the failure message when sha256sum.txt can't be fetched is byte-identical (error: sha256sum.txt download failed). Clean install in 10.8s. Nothing to worry about here.

The checksum fetch can hang for 75s before falling back. This is the one I'd sort before merging. I simulated the case the mirror exists for — mirror reachable, direct GitHub not — by pointing checksum_url at an unroutable address:

[20:35:13] downloading ocr v1.9.5 (darwin/arm64)...
           # binary arrives from the mirror in ~10s, then 75s of silence
[20:36:39] curl: (28) Failed to connect after 75001 ms
[20:36:39] warning: fetching sha256sum.txt from GitHub failed; falling back to mirror "…"
real 1m29.67s

None of the curl calls set a timeout, so that 75s is just the OS TCP default. And since sha256sum.txt sits on the same host as the binary, the users who need the mirror are exactly the ones most likely to fail this request — they pay the full stall, which eats the speedup this PR is for. Worth noting the 1m16s figure in the description was measured before the checksum moved to a direct fetch, so it no longer describes this code path.

With a timeout on that one call:

[20:37:18] curl: (28) Failed to connect after 5002 ms: Timeout was reached
real 20.004s

89.7s → 20.0s, one line:

curl -fsSL --connect-timeout 5 --max-time 15 -o "$tmp/sha256sum.txt" "$checksum_url"

Edge inputs behave as predicted upthread: gh proxy.comhttps://ghproxy.com/… (a different, real domain), https://gh-proxy.comhttps://https://gh-proxy.com/…, trailing slash → //github.com/. Whitespace-only correctly falls back to GitHub.

install.ps1 I couldn't run (no pwsh on this box), so this part is review-only — but two things now look out of sync with the shell script:

  • the warnings still go through Write-Host, while the sh side moved to stderr
  • Invoke-WebRequest has no -TimeoutSec, so the stall above applies on Windows too

The whitespace difference also cuts both ways: .Trim() keeps interior spaces, so gh proxy.com errors out on Windows but silently installs from ghproxy.com on macOS/Linux. Same input, two outcomes.

So: the sh + PS timeouts, the PS stderr consistency, and one real run of install.ps1 on a genuinely restricted network. Four or five lines in total.

@wu21-web
wu21-web marked this pull request as draft August 18, 2026 12:44
@wu21-web wu21-web changed the title feat(installation): support asset download via GITHUB_MIRROR_DOMAIN feat(installation): support asset download via OCR_GITHUB_MIRROR Aug 18, 2026
@wu21-web
wu21-web marked this pull request as ready for review August 18, 2026 15:17
@wu21-web

Copy link
Copy Markdown
Contributor Author

Here are the updates:

  1. In the review, you mentioned api.github.com, the actual source is all github.com, which is bad for Chinese users. So we have to switch both the checksum origin and asset origin to the mirror.
  2. Documentation updates: warn users about the checksum being downloaded for the mirror.
  3. Use [Console]::Error.WriteLine(...
  4. Use curl -fsSL --connect-timeout 5 --max-time 15
  5. pwsh -Command "[ScriptBlock]::Create((Get-Content install.ps1 -Raw)) | Out-Null" passes.

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.

2 participants