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

Fix GitHub token auto discovery #1335

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Fix GitHub token auto discovery #1335

merged 5 commits into from
Sep 1, 2023

Conversation

NobodyXu
Copy link
Member

Fixed #1333

  • Rm dep gh-token since it is broken and we can simply run gh auth token in cargo-binstall instead.
  • binstalk-downloader: Make sure GitHub token is at least 40B long and other than the _, composes of only alphanumeric characters.
  • Warn on failure to read git/credential files
  • Optimize try_from_home to avoid heap allocation of PathBuf

Fixed #1333

 - Rm dep `gh-token` since it is broken and we can simply run
   `gh auth token` in `cargo-binstall` instead.
 - binstalk-downloader: Make sure GitHub token is at least 40B long
   and other than the `_`, composes of only alphanumeric characters.
 - Warn on failure to read `git/credential` files
 - Optimize `try_from_home` to avoid heap allocation of `PathBuf`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@simonsan
Copy link

simonsan commented Aug 31, 2023

  • Rm dep gh-token since it is broken and we can simply run gh auth token in cargo-binstall instead.

That assumes someone has installed the gh cli tool locally, right?

Maybe it would be good to keep that logic, in case someone hasn't installed it and test for the presence of gh cli and if present, use it?

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Member Author

@simonsan If gh isn't available, this PR would simply log a warning message and continue.

I don't see why gh-token should be kept, if gh is not installed then there's likely no gh token (unless it's a leftover from uninstalled gh), so gh-token would also return an Err.

@NobodyXu NobodyXu requested a review from passcod August 31, 2023 13:49
@NobodyXu
Copy link
Member Author

@simonsan Can you try this PR and see if it fixed the bug for you please?
You can download the pre-built artifacts here

@simonsan
Copy link

@simonsan Can you try this PR and see if it fixed the bug for you please? You can download the pre-built artifacts here

PS C:\Users\dailyuse> cargo binstall cargo-audit --force -y
 WARN Failed to read git credential file C:\Users\dailyuse\.git-credentials err=Os { code: 2, kind: NotFound, message: "The system cannot find the file specified." }
 INFO resolve: Resolving package: 'cargo-audit'
 WARN Failed to send quickinstall report for package cargo-audit-0.18.1-x86_64-pc-windows-msvc: Failed to download from remote: could not HEAD https://warehouse-clerk-tmp.vercel.app/api/crate/cargo-audit-0.18.1-x86_64-pc-windows-msvc.tar.gz: HTTP status server error (500 Internal Server Error) for url (https://warehouse-clerk-tmp.vercel.app/api/crate/cargo-audit-0.18.1-x86_64-pc-windows-msvc.tar.gz)
 WARN The package cargo-audit v0.18.1 will be downloaded from third-party source QuickInstall
 INFO This will install the following binaries:
 INFO   - cargo-audit.exe (cargo-audit.exe -> C:\Users\dailyuse\.cargo\bin\cargo-audit.exe)
 INFO Installing binaries...
 INFO Done in 191.8086427s

It throws a few warnings, but it works now. Still takes 3 Minutes though.

@NobodyXu
Copy link
Member Author

It throws a few warnings, but it works now. Still takes 3 Minutes though.

Given that Github token is provided, I suspect this might be a network issue, or maybe related to use of http3.

Can u try building this branch locally without http3 please?

@simonsan
Copy link

It throws a few warnings, but it works now. Still takes 3 Minutes though.

Given that Github token is provided, I suspect this might be a network issue, or maybe related to use of http3.

Can u try building this branch locally without http3 please?

PS C:\Users\dailyuse> cargo binstall cargo-audit --force -y
 INFO resolve: Resolving package: 'cargo-audit'
 WARN The package cargo-audit v0.18.1 will be downloaded from third-party source QuickInstall
 INFO This will install the following binaries:
 INFO   - cargo-audit.exe (cargo-audit.exe -> C:\Users\dailyuse\.cargo\bin\cargo-audit.exe)
 INFO Installing binaries...
 INFO Done in 158.4539701s

With GITHUB_TOKEN env var, still want me to build without HTTP/3?

@simonsan
Copy link

simonsan commented Sep 1, 2023

PS C:\Users\dailyuse\dev-src\cargo-binstall> cargo run -p cargo-binstall -- cargo-audit --force -y
    Finished dev [unoptimized + debuginfo] target(s) in 0.39s
     Running `C:\.cargo-target\debug\cargo-binstall.exe cargo-audit --force -y`
 INFO resolve: Resolving package: 'cargo-audit'
 WARN The package cargo-audit v0.18.1 will be downloaded from third-party source QuickInstall
 INFO This will install the following binaries:
 INFO   - cargo-audit.exe (cargo-audit.exe -> C:\Users\dailyuse\.cargo\bin\cargo-audit.exe)
 INFO Installing binaries...
 INFO Done in 182.9846414s

This PR with GITHUB_TOKEN set.

@NobodyXu
Copy link
Member Author

NobodyXu commented Sep 1, 2023

With GITHUB_TOKEN env var, still want me to build without HTTP/3?

Just want to check if it's HTTP/3 causing slowdown

@simonsan
Copy link

simonsan commented Sep 1, 2023

With GITHUB_TOKEN env var, still want me to build without HTTP/3?

Just want to check if it's HTTP/3 causing slowdown

The one after that comment built with default features, so no HTTP/3 and has still taken 3 minutes.

@NobodyXu NobodyXu added this pull request to the merge queue Sep 1, 2023
@NobodyXu
Copy link
Member Author

NobodyXu commented Sep 1, 2023

The one after that comment built with default features, so no HTTP/3 and has still taken 3 minutes.

Not sure what is wrong then.

Can it be that GitHub rate limits your IP or Github token?

Merged via the queue into main with commit 8a08cdd Sep 1, 2023
26 checks passed
@NobodyXu NobodyXu deleted the feat/gh-token branch September 1, 2023 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants