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

Update submodule URL to use the "Smart HTTP" protocol instead of the "SSH" protocol #137

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

nickpresta
Copy link
Contributor

Hello,

I wanted to propose a small update that changes the way we interact with the libduckdb-sys/duckdb-sources submodule.

Currently, we're using git@github.com:duckdb/duckdb.git as the URL, which requires the SSH protocol. The SSH protocol isn't available everywhere (e.g. due to firewalls) and/or may not be configured with a user with permissions at all.

Instead, we should swap to the "Smart HTTP" protocol, which works just as well for a public repository that we only ever need to read from (e.g. we don't push to it).

This unblocks me from building in CI where access is severely restricted.

Thanks!

@wangfenjin
Copy link
Collaborator

actually unless we are trying to upgrade duckdb source code, we don’t need to access the repo as we already checkin the source as zip file.

is there anything misunderstanding? Or you always want to use the latest duckdb code?

@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #137 (cd9467a) into main (c238951) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage   59.41%   59.41%           
=======================================
  Files          26       26           
  Lines        1481     1481           
=======================================
  Hits          880      880           
  Misses        601      601           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nickpresta
Copy link
Contributor Author

nickpresta commented Mar 17, 2023

It looks like the submodule was added in #127 so that updating would be easier.

I'm running into an issue when trying to cargo build, which then tries to update the submodule:

Cargo.toml

[package]
name = "test"
version = "0.1.0"
edition = "2021"

[dependencies]
duckdb = { git = "https://github.com/wangfenjin/duckdb-rs.git", features = ["httpfs", "parquet"] }
$ cargo build
    Updating git repository `https://github.com/wangfenjin/duckdb-rs.git`
    Updating git submodule `git@github.com:duckdb/duckdb.git`
remote: Enumerating objects: 321292, done.
remote: Counting objects: 100% (891/891), done.
remote: Compressing objects: 100% (399/399), done.
remote: Total 321292 (delta 579), reused 724 (delta 478), pack-reused 320401
Receiving objects: 100% (321292/321292), 186.89 MiB | 30.19 MiB/s, done.
Resolving deltas: 100% (263773/263773), done.
From github.com:duckdb/duckdb
 * [new branch]            master        -> origin/master
 * [new ref]                             -> origin/HEAD
 * [new tag]               master-builds -> master-builds
 * [new tag]               v0.1.0        -> v0.1.0
 * [new tag]               v0.1.1        -> v0.1.1
 * [new tag]               v0.1.2        -> v0.1.2
 * [new tag]               v0.1.3        -> v0.1.3
 * [new tag]               v0.1.4        -> v0.1.4
 * [new tag]               v0.1.5        -> v0.1.5
 * [new tag]               v0.1.6        -> v0.1.6
 * [new tag]               v0.1.7        -> v0.1.7
 * [new tag]               v0.1.8        -> v0.1.8
 * [new tag]               v0.1.9        -> v0.1.9
 * [new tag]               v0.2.0        -> v0.2.0
 * [new tag]               v0.2.1        -> v0.2.1
 * [new tag]               v0.2.2        -> v0.2.2
 * [new tag]               v0.2.3        -> v0.2.3
 * [new tag]               v0.2.4        -> v0.2.4
 * [new tag]               v0.2.5        -> v0.2.5
 * [new tag]               v0.2.6        -> v0.2.6
 * [new tag]               v0.2.7        -> v0.2.7
 * [new tag]               v0.2.8        -> v0.2.8
 * [new tag]               v0.2.9        -> v0.2.9
 * [new tag]               v0.3.0        -> v0.3.0
 * [new tag]               v0.3.1        -> v0.3.1
 * [new tag]               v0.3.2        -> v0.3.2
 * [new tag]               v0.3.3        -> v0.3.3
 * [new tag]               v0.3.4        -> v0.3.4
 * [new tag]               v0.4.0        -> v0.4.0
 * [new tag]               v0.5.0        -> v0.5.0
 * [new tag]               v0.5.1        -> v0.5.1
 * [new tag]               v0.6.0        -> v0.6.0
 * [new tag]               v0.6.1        -> v0.6.1
 * [new tag]               v0.7.0        -> v0.7.0
 * [new tag]               v0.7.1        -> v0.7.1

The issue is that even though I've specified the duckdb dependency with https://, the submodule is being fetched via git@github.com, which fails.

You can reproduce this by deleting ~/.cargo/git/checkouts/duckdb-rs* and then running cargo build and seeing that the submodule is trying to be fetched.

When running with my change, we see:

$ cargo build --verbose
    Updating git repository `https://github.com/nickpresta/duckdb-rs.git`
     Running `git fetch --verbose --force --update-head-ok 'https://github.com/nickpresta/duckdb-rs.git' '+HEAD:refs/remotes/origin/HEAD'`
POST git-upload-pack (305 bytes)
From https://github.com/nickpresta/duckdb-rs
 = [up to date]                 -> origin/HEAD
    Updating git submodule `https://github.com/duckdb/duckdb.git`
     Running `git fetch --tags --verbose --force --update-head-ok 'https://github.com/duckdb/duckdb.git' '+refs/heads/*:refs/remotes/origin/*' '+HEAD:refs/remotes/origin/HEAD'`
... MORE OUTPUT HERE ...

that we're now using https:// for the repo and the submodule.

@wangfenjin
Copy link
Collaborator

rust-lang/cargo#10717

I think we need to do this

@wangfenjin
Copy link
Collaborator

Could you help make the change?

@nickpresta
Copy link
Contributor Author

Thanks for finding that issue. I pushed a new commit with the update = none configuration and now I see:

Skipping git submodule `git@github.com:duckdb/duckdb.git` due to update strategy in .gitmodules

I can confirm that this fixes the issue within my CI environment (and cargo build is now faster 😃)

@wangfenjin
Copy link
Collaborator

Thanks!

@wangfenjin wangfenjin merged commit d231b27 into duckdb:main Mar 17, 2023
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